From b2be548275c99cd061ec1c2f9c27eedae64a17b8 Mon Sep 17 00:00:00 2001
From: TowyTowy <85077986+TowyTowy@users.noreply.github.com>
Date: Sun, 12 Jul 2026 01:25:31 +0200
Subject: [PATCH] reverseproxy: fix broken reservoir sampling in random_choose
 policy (#7873)

The random_choose selection policy is meant to implement
power-of-d-choices: sample d available upstreams uniformly, then pick
the least-loaded of that sample. The sampling loop, however, was not a
correct reservoir sample (Algorithm R): it never filled the first k
reservoir slots unconditionally, instead writing every candidate to a
random slot j = rand(i+1), which can evict an earlier candidate while
leaving another slot nil. It also derived j from the upstream's index
in the pool rather than from the number of available upstreams seen,
skewing the sample whenever unavailable upstreams precede available
ones.

As a result the reservoir frequently held fewer than min(k, available)
upstreams, so the least-load comparison often never happened. Most
notably, with two upstreams and 'random_choose 2' (the canonical
power-of-two-choices setup), half of all requests were routed to the
more-loaded upstream even when it was saturated and the other idle --
identical behavior to plain 'random'. The nil slots this leaves behind
were also the cause of the panic reported in #3810, which was patched
by skipping nils in leastRequests rather than by fixing the sampling.

Replace the loop with a standard Algorithm R reservoir sample over the
available upstreams: fill the first k slots, then replace a random slot
with probability k/n. Every available upstream is now sampled uniformly
and the reservoir always holds min(k, available) candidates.

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
---
 .../reverseproxy/selectionpolicies.go         | 17 +++++++++---
 .../reverseproxy/selectionpolicies_test.go    | 27 +++++++++++++++++++
 2 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/modules/caddyhttp/reverseproxy/selectionpolicies.go b/modules/caddyhttp/reverseproxy/selectionpolicies.go
index 86a3d0d7c..83f8f8a07 100644
--- a/modules/caddyhttp/reverseproxy/selectionpolicies.go
+++ b/modules/caddyhttp/reverseproxy/selectionpolicies.go
@@ -233,12 +233,23 @@ func (r RandomChoiceSelection) Validate() error {
 // Select returns an available host, if any.
 func (r RandomChoiceSelection) Select(pool UpstreamPool, _ *http.Request, _ http.ResponseWriter) *Upstream {
 	k := min(r.Choose, len(pool))
-	choices := make([]*Upstream, k)
-	for i, upstream := range pool {
+
+	// reservoir sampling (Algorithm R) over the available upstreams:
+	// the first k available upstreams fill the reservoir, then each
+	// subsequent one replaces a random reservoir entry with probability
+	// k/n, so every available upstream is sampled uniformly
+	choices := make([]*Upstream, 0, k)
+	var available int
+	for _, upstream := range pool {
 		if !upstream.Available() {
 			continue
 		}
-		j := weakrand.IntN(i + 1) //nolint:gosec
+		available++
+		if len(choices) < k {
+			choices = append(choices, upstream)
+			continue
+		}
+		j := weakrand.IntN(available) //nolint:gosec
 		if j < k {
 			choices[j] = upstream
 		}
diff --git a/modules/caddyhttp/reverseproxy/selectionpolicies_test.go b/modules/caddyhttp/reverseproxy/selectionpolicies_test.go
index 7c912ce01..84fd4493c 100644
--- a/modules/caddyhttp/reverseproxy/selectionpolicies_test.go
+++ b/modules/caddyhttp/reverseproxy/selectionpolicies_test.go
@@ -725,6 +725,33 @@ func TestRandomChoicePolicy(t *testing.T) {
 	}
 }
 
+func TestRandomChoicePolicyLeastLoaded(t *testing.T) {
+	// when the number of available upstreams does not exceed the choose
+	// count, all of them must be candidates, so the least-loaded one
+	// must always be selected; the pool intentionally starts with an
+	// unavailable upstream to verify that reservoir sampling counts
+	// available upstreams rather than pool indices
+	pool := testPool()
+	pool[0].Dial = "localhost:8080"
+	pool[1].Dial = "localhost:8081"
+	pool[2].Dial = "localhost:8082"
+	pool[0].setHealthy(false)
+	pool[1].setHealthy(true)
+	pool[2].setHealthy(true)
+	pool[1].countRequest(30)
+	// pool[2] has no active requests
+
+	request := httptest.NewRequest(http.MethodGet, "/test", nil)
+	randomChoicePolicy := RandomChoiceSelection{Choose: 2}
+
+	for i := 0; i < 100; i++ {
+		h := randomChoicePolicy.Select(pool, request, nil)
+		if h != pool[2] {
+			t.Fatalf("with 2 available upstreams and choose=2, the least-loaded upstream (pool[2]) must always be selected; got %v on iteration %d", h, i)
+		}
+	}
+}
+
 func TestCookieHashPolicy(t *testing.T) {
 	ctx, cancel := caddy.NewContext(caddy.Context{Context: context.Background()})
 	defer cancel()