fix: don't double-count webhook capacity reservations in PercentageRunnersBusy

controllers/actions.summerwind.net/autoscaling.go

@@ -23,7 +23,7 @@ const (
 	defaultScaleDownFactor    = 0.7
 )
 
-func (r *HorizontalRunnerAutoscalerReconciler) suggestDesiredReplicas(ghc *arcgithub.Client, st scaleTarget, hra v1alpha1.HorizontalRunnerAutoscaler) (*int, error) {
+func (r *HorizontalRunnerAutoscalerReconciler) suggestDesiredReplicas(ghc *arcgithub.Client, st scaleTarget, hra v1alpha1.HorizontalRunnerAutoscaler, reserved int) (*int, error) {
 	if hra.Spec.MinReplicas == nil {
 		return nil, fmt.Errorf("horizontalrunnerautoscaler %s/%s is missing minReplicas", hra.Namespace, hra.Name)
 	} else if hra.Spec.MaxReplicas == nil {
@@ -52,7 +52,7 @@ func (r *HorizontalRunnerAutoscalerReconciler) suggestDesiredReplicas(ghc *arcgi
 	case v1alpha1.AutoscalingMetricTypeTotalNumberOfQueuedAndInProgressWorkflowRuns:
 		suggested, err = r.suggestReplicasByQueuedAndInProgressWorkflowRuns(ghc, st, hra, &primaryMetric)
 	case v1alpha1.AutoscalingMetricTypePercentageRunnersBusy:
-		suggested, err = r.suggestReplicasByPercentageRunnersBusy(ghc, st, hra, primaryMetric)
+		suggested, err = r.suggestReplicasByPercentageRunnersBusy(ghc, st, hra, primaryMetric, reserved)
 	default:
 		return nil, fmt.Errorf("validating autoscaling metrics: unsupported metric type %q", primaryMetric.Type)
 	}
@@ -242,7 +242,7 @@ func (r *HorizontalRunnerAutoscalerReconciler) suggestReplicasByQueuedAndInProgr
 	return &necessaryReplicas, nil
 }
 
-func (r *HorizontalRunnerAutoscalerReconciler) suggestReplicasByPercentageRunnersBusy(ghc *arcgithub.Client, st scaleTarget, hra v1alpha1.HorizontalRunnerAutoscaler, metrics v1alpha1.MetricSpec) (*int, error) {
+func (r *HorizontalRunnerAutoscalerReconciler) suggestReplicasByPercentageRunnersBusy(ghc *arcgithub.Client, st scaleTarget, hra v1alpha1.HorizontalRunnerAutoscaler, metrics v1alpha1.MetricSpec, reserved int) (*int, error) {
 	ctx := context.Background()
 	scaleUpThreshold := defaultScaleUpThreshold
 	scaleDownThreshold := defaultScaleDownThreshold
@@ -328,6 +328,12 @@ func (r *HorizontalRunnerAutoscalerReconciler) suggestReplicasByPercentageRunner
 		desiredReplicasBefore = *v
 	}
 
+	// Exclude reserved capacity: the caller re-adds it, so leaving it in double-counts (#1962).
+	desiredReplicasBefore -= reserved
+	if desiredReplicasBefore < 1 {
+		desiredReplicasBefore = 1
+	}
+
 	var (
 		numRunners           int
 		numRunnersRegistered int
@@ -376,8 +382,14 @@ func (r *HorizontalRunnerAutoscalerReconciler) suggestReplicasByPercentageRunner
 		numTerminatingBusy++
 	}
 
+	// Same reasoning as desiredReplicasBefore above.
+	busy := numRunnersBusy + numTerminatingBusy - reserved
+	if busy < 0 {
+		busy = 0
+	}
+
 	var desiredReplicas int
-	fractionBusy := float64(numRunnersBusy+numTerminatingBusy) / float64(desiredReplicasBefore)
+	fractionBusy := float64(busy) / float64(desiredReplicasBefore)
 	if fractionBusy >= scaleUpThreshold {
 		if scaleUpAdjustment > 0 {
 			desiredReplicas = desiredReplicasBefore + scaleUpAdjustment
@@ -391,7 +403,7 @@ func (r *HorizontalRunnerAutoscalerReconciler) suggestReplicasByPercentageRunner
 			desiredReplicas = int(float64(desiredReplicasBefore) * scaleDownFactor)
 		}
 	} else {
-		desiredReplicas = *st.replicas
+		desiredReplicas = desiredReplicasBefore
 	}
 
 	// NOTES for operators:

controllers/actions.summerwind.net/autoscaling_test.go

@@ -6,13 +6,15 @@ import (
 	"net/http/httptest"
 	"net/url"
 	"testing"
+	"time"
 
 	"github.com/actions/actions-runner-controller/apis/actions.summerwind.net/v1alpha1"
 	"github.com/actions/actions-runner-controller/github"
 	"github.com/actions/actions-runner-controller/github/fake"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	clientgoscheme "k8s.io/client-go/kubernetes/scheme"
+	ctrlclientfake "sigs.k8s.io/controller-runtime/pkg/client/fake"
 	"sigs.k8s.io/controller-runtime/pkg/log/zap"
 )
 
@@ -437,6 +439,97 @@ func TestDetermineDesiredReplicas_RepositoryRunner(t *testing.T) {
 	}
 }
 
+// TestDetermineDesiredReplicas_PercentageRunnersBusyWithWebhookReservation reproduces
+// https://github.com/actions/actions-runner-controller/issues/1962: combining the
+// PercentageRunnersBusy metric with an active webhook CapacityReservation must not
+// double-count the reservation and compound the replica count on every reconcile.
+func TestDetermineDesiredReplicas_PercentageRunnersBusyWithWebhookReservation(t *testing.T) {
+	intPtr := func(v int) *int {
+		return &v
+	}
+
+	log := zap.New(func(o *zap.Options) {
+		o.Development = true
+	})
+
+	scheme := runtime.NewScheme()
+	_ = clientgoscheme.AddToScheme(scheme)
+	_ = v1alpha1.AddToScheme(scheme)
+
+	server := fake.NewServer(
+		fake.WithListRunnersResponse(200, fake.RunnersListBody),
+	)
+	defer server.Close()
+	ghc := newGithubClient(server)
+
+	h := &HorizontalRunnerAutoscalerReconciler{
+		Log:                   log,
+		Scheme:                scheme,
+		Client:                ctrlclientfake.NewClientBuilder().WithScheme(scheme).Build(),
+		DefaultScaleDownDelay: DefaultScaleDownDelay,
+	}
+
+	rd := v1alpha1.RunnerDeployment{
+		ObjectMeta: metav1.ObjectMeta{Name: "testrd"},
+		Spec: v1alpha1.RunnerDeploymentSpec{
+			Template: v1alpha1.RunnerTemplate{
+				Spec: v1alpha1.RunnerSpec{
+					RunnerConfig: v1alpha1.RunnerConfig{Repository: "test/valid"},
+				},
+			},
+			Replicas: intPtr(1),
+		},
+	}
+
+	hra := v1alpha1.HorizontalRunnerAutoscaler{
+		Spec: v1alpha1.HorizontalRunnerAutoscalerSpec{
+			MinReplicas: intPtr(1),
+			MaxReplicas: intPtr(20),
+			Metrics: []v1alpha1.MetricSpec{
+				{Type: v1alpha1.AutoscalingMetricTypePercentageRunnersBusy},
+			},
+			// A webhook-triggered reservation for one queued job's worth of capacity,
+			// still active across every reconcile below.
+			CapacityReservations: []v1alpha1.CapacityReservation{
+				{
+					Name:           "queued-job",
+					Replicas:       3,
+					ExpirationTime: metav1.NewTime(time.Now().Add(time.Hour)),
+				},
+			},
+		},
+	}
+
+	now := time.Now()
+
+	// minReplicas + the reservation, counted exactly once.
+	want := *hra.Spec.MinReplicas + 3
+
+	var got int
+	for i := 0; i < 5; i++ {
+		minReplicas, _, _, err := h.getMinReplicas(log, now, hra)
+		if err != nil {
+			t.Fatalf("iteration %d: unexpected error: %v", i, err)
+		}
+
+		st := h.scaleTargetFromRD(context.Background(), rd)
+
+		got, err = h.computeReplicasWithCache(ghc, log, now, st, hra, minReplicas)
+		if err != nil {
+			t.Fatalf("iteration %d: unexpected error: %v", i, err)
+		}
+
+		if got != want {
+			t.Errorf("iteration %d: got %d desired replicas, want %d (minReplicas+reserved, stable) - a growing sequence means the reservation is being double-counted (#1962)", i, got, want)
+		}
+
+		// Simulate the controller patching the RunnerDeployment to the newly computed
+		// replica count before the next reconcile, exactly as Reconcile() does.
+		rd.Spec.Replicas = intPtr(got)
+		hra.Status.DesiredReplicas = intPtr(got)
+	}
+}
+
 func TestDetermineDesiredReplicas_OrganizationalRunner(t *testing.T) {
 	intPtr := func(v int) *int {
 		return &v

controllers/actions.summerwind.net/horizontalrunnerautoscaler_controller.go

@@ -475,9 +475,18 @@ func (r *HorizontalRunnerAutoscalerReconciler) getMinReplicas(log logr.Logger, n
 }
 
 func (r *HorizontalRunnerAutoscalerReconciler) computeReplicasWithCache(ghc *arcgithub.Client, log logr.Logger, now time.Time, st scaleTarget, hra v1alpha1.HorizontalRunnerAutoscaler, minReplicas int) (int, error) {
+	var reserved int
+
+	for _, reservation := range hra.Spec.CapacityReservations {
+		if reservation.ExpirationTime.After(now) {
+			reserved += reservation.Replicas
+		}
+	}
+
+	// reserved is passed in so suggesters can exclude it from their own baseline (#1962).
 	var suggestedReplicas int
 
-	v, err := r.suggestDesiredReplicas(ghc, st, hra)
+	v, err := r.suggestDesiredReplicas(ghc, st, hra, reserved)
 	if err != nil {
 		return 0, err
 	}
@@ -488,14 +497,6 @@ func (r *HorizontalRunnerAutoscalerReconciler) computeReplicasWithCache(ghc *arc
 		suggestedReplicas = *v
 	}
 
-	var reserved int
-
-	for _, reservation := range hra.Spec.CapacityReservations {
-		if reservation.ExpirationTime.After(now) {
-			reserved += reservation.Replicas
-		}
-	}
-
 	newDesiredReplicas := suggestedReplicas + reserved
 
 	if newDesiredReplicas < minReplicas {