From 11e1a443d080fe9dcd417d3156f1743b02c23b2d Mon Sep 17 00:00:00 2001 From: Victor Nazzaro Date: Mon, 27 Jul 2026 13:33:49 -0700 Subject: [PATCH] Force-remove EphemeralRunner registration finalizer after deletion timeout When an EphemeralRunner is deleting and the Actions service keeps returning JobStillRunningError, the reconciler requeues forever. If the runner pod is already gone or in a terminal phase and deletion has been pending past a timeout (default 10m, injectable for tests), force-remove the registration finalizer so the CR can finish deleting. Fixes stuck Terminating EphemeralRunners after mid-job pod death (eviction, OOM, node loss). Related: #4155, #4307 Signed-off-by: Victor Nazzaro --- .../ephemeralrunner_controller.go | 61 +++++- .../ephemeralrunner_controller_test.go | 187 ++++++++++++++++++ 2 files changed, 245 insertions(+), 3 deletions(-) diff --git a/controllers/actions.github.com/ephemeralrunner_controller.go b/controllers/actions.github.com/ephemeralrunner_controller.go index e78ede68ff..f0d11ca569 100644 --- a/controllers/actions.github.com/ephemeralrunner_controller.go +++ b/controllers/actions.github.com/ephemeralrunner_controller.go @@ -46,6 +46,10 @@ const ( ephemeralRunnerActionsFinalizerName = "ephemeralrunner.actions.github.com/runner-registration-finalizer" ) +// how long a deleting EphemeralRunner may stay blocked on JobStillRunning +// before the registration finalizer is force-removed +const defaultRegistrationFinalizerForceTimeout = 10 * time.Minute + // EphemeralRunnerReconciler reconciles a EphemeralRunner object type EphemeralRunnerReconciler struct { client.Client @@ -53,6 +57,16 @@ type EphemeralRunnerReconciler struct { Scheme *runtime.Scheme PublishMetrics bool ResourceBuilder + + // RegistrationFinalizerForceTimeout overrides defaultRegistrationFinalizerForceTimeout when set + RegistrationFinalizerForceTimeout time.Duration +} + +func (r *EphemeralRunnerReconciler) registrationFinalizerForceTimeout() time.Duration { + if r.RegistrationFinalizerForceTimeout > 0 { + return r.RegistrationFinalizerForceTimeout + } + return defaultRegistrationFinalizerForceTimeout } var ephemeralRunnerPhaseMetrics = struct { @@ -110,12 +124,28 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ log.Error(err, "Failed to clean up runner from service") return ctrl.Result{}, err } + forced := false if !ok { - log.Info("Runner is not finished yet, retrying in 30s") - return ctrl.Result{RequeueAfter: 30 * time.Second}, nil + force, forceErr := r.shouldForceRegistrationFinalizerRemoval(ctx, &ephemeralRunner, log) + if forceErr != nil { + log.Error(forceErr, "Failed to evaluate registration finalizer force-removal") + return ctrl.Result{}, forceErr + } + if !force { + log.Info("Runner is not finished yet, retrying in 30s") + return ctrl.Result{RequeueAfter: 30 * time.Second}, nil + } + forced = true + log.Info( + "Actions service still reports the job as running, but the runner pod is gone or terminal and deletion exceeded the timeout; force-removing registration finalizer", + "timeout", r.registrationFinalizerForceTimeout(), + "deletionTimestamp", ephemeralRunner.DeletionTimestamp, + ) } - log.Info("Runner is cleaned up from the service, removing finalizer") + if !forced { + log.Info("Runner is cleaned up from the service, removing finalizer") + } if controllerutil.RemoveFinalizer(&ephemeralRunner, ephemeralRunnerActionsFinalizerName) { log.Info("Removed finalizer from ephemeral runner") if err := r.Patch(ctx, &ephemeralRunner, client.MergeFrom(original)); err != nil { @@ -469,6 +499,31 @@ func (r *EphemeralRunnerReconciler) cleanupRunnerFromService(ctx context.Context return true, nil } +// shouldForceRegistrationFinalizerRemoval returns true when the registration finalizer can be removed without a successful RemoveRunner call: +// the runner has been deleting for longer than the timeout and its pod is gone or in a terminal phase, so it can never unregister on its own. +func (r *EphemeralRunnerReconciler) shouldForceRegistrationFinalizerRemoval(ctx context.Context, ephemeralRunner *v1alpha1.EphemeralRunner, log logr.Logger) (bool, error) { + if ephemeralRunner.DeletionTimestamp.IsZero() { + return false, nil + } + if time.Since(ephemeralRunner.DeletionTimestamp.Time) < r.registrationFinalizerForceTimeout() { + return false, nil + } + + pod := new(corev1.Pod) + err := r.Get(ctx, types.NamespacedName{Namespace: ephemeralRunner.Namespace, Name: ephemeralRunner.Name}, pod) + switch { + case kerrors.IsNotFound(err): + return true, nil + case err != nil: + return false, fmt.Errorf("failed to get runner pod while evaluating finalizer force-removal: %w", err) + case pod.Status.Phase == corev1.PodSucceeded || pod.Status.Phase == corev1.PodFailed: + log.Info("Runner pod is in a terminal phase and cannot unregister on its own", "phase", pod.Status.Phase) + return true, nil + default: + return false, nil + } +} + func (r *EphemeralRunnerReconciler) cleanupResources(ctx context.Context, ephemeralRunner *v1alpha1.EphemeralRunner, log logr.Logger) error { log.Info("Cleaning up the runner pod") pod := new(corev1.Pod) diff --git a/controllers/actions.github.com/ephemeralrunner_controller_test.go b/controllers/actions.github.com/ephemeralrunner_controller_test.go index 80c27134a7..97e7687065 100644 --- a/controllers/actions.github.com/ephemeralrunner_controller_test.go +++ b/controllers/actions.github.com/ephemeralrunner_controller_test.go @@ -1285,6 +1285,193 @@ var _ = Describe("EphemeralRunner", func() { }) }) + Describe("Registration finalizer force removal", func() { + var ctx context.Context + var mgr ctrl.Manager + var autoscalingNS *corev1.Namespace + var configSecret *corev1.Secret + var controller *EphemeralRunnerReconciler + var ephemeralRunner *v1alpha1.EphemeralRunner + var forceTimeout time.Duration + + BeforeEach(func() { + ctx = context.Background() + autoscalingNS, mgr = createNamespace(GinkgoT(), k8sClient) + configSecret = createDefaultSecret(GinkgoT(), k8sClient, autoscalingNS.Name) + }) + + JustBeforeEach(func() { + controller = &EphemeralRunnerReconciler{ + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + Log: logf.Log, + RegistrationFinalizerForceTimeout: forceTimeout, + ResourceBuilder: ResourceBuilder{ + SecretResolver: secretresolver.New(mgr.GetClient(), scalefake.NewMultiClient( + scalefake.WithClient( + scalefake.NewClient( + scalefake.WithGenerateJitRunnerConfig( + &scaleset.RunnerScaleSetJitRunnerConfig{ + Runner: &scaleset.RunnerReference{ID: 1, Name: "test-runner"}, + EncodedJITConfig: "fake-jit-config", + }, + nil, + ), + scalefake.WithRemoveRunner(scaleset.JobStillRunningError), + ), + ), + )), + }, + } + + err := controller.SetupWithManager(mgr) + Expect(err).To(BeNil(), "failed to setup controller") + + ephemeralRunner = newExampleRunner("test-runner", autoscalingNS.Name, configSecret.Name) + err = k8sClient.Create(ctx, ephemeralRunner) + Expect(err).To(BeNil(), "failed to create ephemeral runner") + + startManagers(GinkgoT(), mgr) + + Eventually( + func() ([]string, error) { + er := new(v1alpha1.EphemeralRunner) + if err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, er); err != nil { + return nil, err + } + n := len(er.Finalizers) + return er.Finalizers[:n:n], nil + }, + ephemeralRunnerTimeout, + ephemeralRunnerInterval, + ).Should(ContainElements(ephemeralRunnerFinalizerName, ephemeralRunnerActionsFinalizerName), "both finalizers should be added") + + Eventually( + func() error { + return k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, new(corev1.Pod)) + }, + ephemeralRunnerTimeout, + ephemeralRunnerInterval, + ).Should(Succeed(), "runner pod should be created") + }) + + deleteEphemeralRunnerAndWaitForTerminating := func() { + err := k8sClient.Delete(ctx, ephemeralRunner) + Expect(err).To(BeNil(), "failed to delete ephemeral runner") + + Eventually( + func() (bool, error) { + er := new(v1alpha1.EphemeralRunner) + if err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, er); err != nil { + return false, err + } + return !er.DeletionTimestamp.IsZero(), nil + }, + ephemeralRunnerTimeout, + ephemeralRunnerInterval, + ).Should(BeTrue(), "ephemeral runner should be terminating but held by finalizers") + } + + Context("with a short force timeout", func() { + BeforeEach(func() { + forceTimeout = time.Second + }) + + It("force-removes the registration finalizer when the pod is gone and deletion exceeded the timeout", func() { + deleteEphemeralRunnerAndWaitForTerminating() + + Consistently( + func() error { + return k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, new(v1alpha1.EphemeralRunner)) + }, + 2*time.Second, + ephemeralRunnerInterval, + ).Should(Succeed(), "ephemeral runner should stay terminating while the pod exists") + + pod := new(corev1.Pod) + err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, pod) + Expect(err).To(BeNil(), "failed to get runner pod") + err = k8sClient.Delete(ctx, pod) + Expect(err).To(BeNil(), "failed to delete runner pod") + + Eventually( + func() bool { + err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, new(v1alpha1.EphemeralRunner)) + return kerrors.IsNotFound(err) + }, + ephemeralRunnerTimeout, + ephemeralRunnerInterval, + ).Should(BeTrue(), "ephemeral runner should be force-finalized and deleted") + }) + + It("force-removes the registration finalizer when the pod is in a terminal phase and deletion exceeded the timeout", func() { + deleteEphemeralRunnerAndWaitForTerminating() + + Consistently( + func() error { + return k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, new(v1alpha1.EphemeralRunner)) + }, + 2*time.Second, + ephemeralRunnerInterval, + ).Should(Succeed(), "ephemeral runner should stay terminating while the pod is not terminal") + + pod := new(corev1.Pod) + err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, pod) + Expect(err).To(BeNil(), "failed to get runner pod") + pod.Status.Phase = corev1.PodFailed + err = k8sClient.Status().Update(ctx, pod) + Expect(err).To(BeNil(), "failed to update pod status") + + Eventually( + func() bool { + err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, new(v1alpha1.EphemeralRunner)) + return kerrors.IsNotFound(err) + }, + ephemeralRunnerTimeout, + ephemeralRunnerInterval, + ).Should(BeTrue(), "ephemeral runner should be force-finalized and deleted") + }) + }) + + Context("with a long force timeout", func() { + BeforeEach(func() { + forceTimeout = 5 * time.Minute + }) + + It("does not force-remove the registration finalizer before the timeout even when the pod is gone", func() { + deleteEphemeralRunnerAndWaitForTerminating() + + pod := new(corev1.Pod) + err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, pod) + Expect(err).To(BeNil(), "failed to get runner pod") + err = k8sClient.Delete(ctx, pod) + Expect(err).To(BeNil(), "failed to delete runner pod") + + Eventually( + func() bool { + err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, new(corev1.Pod)) + return kerrors.IsNotFound(err) + }, + ephemeralRunnerTimeout, + ephemeralRunnerInterval, + ).Should(BeTrue(), "runner pod should be gone") + + Consistently( + func() ([]string, error) { + er := new(v1alpha1.EphemeralRunner) + if err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, er); err != nil { + return nil, err + } + n := len(er.Finalizers) + return er.Finalizers[:n:n], nil + }, + 3*time.Second, + ephemeralRunnerInterval, + ).Should(ContainElement(ephemeralRunnerActionsFinalizerName), "registration finalizer should be kept before the timeout") + }) + }) + }) + Describe("Pod proxy config", func() { var ctx context.Context var mgr ctrl.Manager