Bound a probe failure's weight to a window of probes

A backend whose health_probe_path is wrong rejects every probe, so no success can ever arrive to clear the sticky run-failed flag. One transient failure in the middle of those rejections excluded such a backend from the pool for the life of the process, reintroducing the stranding bug. Decide unhealthy vs probe_unsupported on whether a real failure landed within the last health_probe_failures probes, floored at 3 so the window always spans a reject/reject/failure cycle. Sustained alternation keeps a failure in every window and stays unhealthy; an aged-out failure leaves a pure-rejection run on probe_unsupported and back in service.
2026-09-06 00:28:01 +10:00
parent d34782b028
commit 7fd5f72de1
3 changed files with 178 additions and 26 deletions
@@ -251,11 +251,20 @@ not answering.
  blip cannot flap a backend out, and one lucky reply cannot flap it back in.
  The run counts every unsuccessful probe whatever its kind, so a backend that
  fails every probe in mixed ways — a `503`, then a `404`, then a timeout —
-  still trips the threshold; only a success resets the run. The kinds decide
-  *which* state the run enters: a real failure anywhere in the run outranks a
-  refusal, so the backend goes `unhealthy` and is skipped, and only a run of
-  nothing but refusals enters `probe_unsupported` and stays in service. A down
-  backend keeps being probed, so recovery is automatic.
+  still trips the threshold; only a success resets the run. A down backend keeps
+  being probed, so recovery is automatic.
+- **A window decides which state the run enters.** The run is a *failure* run —
+  `unhealthy`, backend skipped — whenever a real failure landed within the last
+  `health_probe_failures` probes, floored at 3 so the window always spans a
+  reject-heavy cycle. A run whose whole window holds nothing but refusals is
+  `probe_unsupported` and keeps serving queries. A backend flipping between kinds
+  — `404`, `503`, `404`, timeout — therefore keeps a failure inside every window
+  and stays out of the pool, while a backend whose probe path is merely wrong
+  returns to service one window after the single transient failure it suffered.
+  Bounding the evidence to a window is what makes that possible: a wrong probe
+  path can never answer a probe successfully, so a failure held against the whole
+  run would be held forever and would strand a backend that is answering queries
+  perfectly. A success clears the window along with the run.
 - **Fails open** — if the prober has marked **every** backend down, `pdbmux`
  queries them all anyway. A wrong `health_probe_path`, a broken prober or a
  partition that only the prober sees can therefore never black-hole traffic;
@@ -19,6 +19,11 @@ const probeBodyLimit = 1 << 20
 // runningState is the healthy state reported by PuppetDB's status service.
 const runningState = "running"

+// minFailureWindow floors the failure window at the shortest reject-heavy cycle
+// that must not flap — reject, reject, failure — since a shorter window would
+// let its failure age out between two rejections.
+const minFailureWindow = 3
+
 const (
 	stateHealthy          = "healthy"
 	stateUnhealthy        = "unhealthy"
@@ -42,13 +47,13 @@ type probeState struct {
 	healthy     bool
 	unsupported bool
 	probed      bool
-	// runFailed records whether the current run of not-OK probes contains at
-	// least one real failure, which decides which non-healthy state it enters.
-	runFailed bool
-	failures  int
-	successes int
-	lastProbe time.Time
-	lastErr   string
+	// failWindow counts the probes over which the last real failure still counts
+	// as evidence; while it is positive a not-OK run is a failure run.
+	failWindow int
+	failures   int
+	successes  int
+	lastProbe  time.Time
+	lastErr    string
 }

 // backendHealth is a copy of one backend's probe state, safe to read outside the
@@ -121,6 +126,7 @@ type prober struct {
 	interval  time.Duration
 	failures  int
 	successes int
+	window    int
 	client    *http.Client
 	log       *log.Logger
 	now       func() time.Time
@@ -143,6 +149,7 @@ func newProber(cfg Config, logger *log.Logger) *prober {
 		interval:  cfg.HealthProbeInterval,
 		failures:  cfg.HealthProbeFailures,
 		successes: cfg.HealthProbeSuccesses,
+		window:    max(cfg.HealthProbeFailures, minFailureWindow),
 		client:    &http.Client{Timeout: cfg.HealthProbeTimeout},
 		log:       logger,
 		now:       time.Now,
@@ -244,7 +251,10 @@ func (p *prober) probeOne(ctx context.Context, b Backend) {
 // threshold is reached so a single blip cannot flap a backend in either
 // direction. One run of consecutive not-OK probes makes the down decision, no
 // matter how the kinds are mixed; the kinds only pick which non-healthy state
-// the run enters, and a real failure anywhere in the run outranks a rejection.
+// the run enters. A real failure outranks a rejection for the length of the
+// failure window, not for the whole run: a backend whose probe path is
+// permanently wrong would otherwise be stranded out of the pool for good by one
+// old transient failure, since only a success it can never give clears it.
 func (p *prober) record(name string, err error) {
 	p.mu.Lock()
 	st, ok := p.states[name]
@@ -259,7 +269,7 @@ func (p *prober) record(name string, err error) {
 	var transition string
 	if outcome == outcomeOK {
 		st.lastErr = ""
-		st.failures, st.runFailed = 0, false
+		st.failures, st.failWindow = 0, 0
 		st.successes++
 		if st.successes >= p.successes {
 			switch {
@@ -275,11 +285,15 @@ func (p *prober) record(name string, err error) {
 		st.lastErr = err.Error()
 		st.successes = 0
 		st.failures++
-		st.runFailed = st.runFailed || outcome == outcomeFailure
+		if outcome == outcomeFailure {
+			st.failWindow = p.window
+		} else if st.failWindow > 0 {
+			st.failWindow--
+		}
 		if st.failures >= p.failures {
 			switch {
-			case st.runFailed:
-				// Some probe in this run was evidence about the backend itself, so the
+			case st.failWindow > 0:
+				// A probe inside the window was evidence about the backend itself, so the
 				// run is a failure run whatever else it also contains.
 				st.unsupported = false
 				if st.healthy {
@@ -4,6 +4,7 @@ import (
 	"context"
 	"encoding/json"
 	"errors"
+	"fmt"
 	"io"
 	"log"
 	"net/http"
@@ -712,28 +713,156 @@ func TestProber_RecoversFromProbeUnsupported(t *testing.T) {
 	}
 }

-// Rejections cannot talk a down backend back into the pool while its failure run
-// is unbroken — the failures are still the run's best evidence. A successful
-// probe ends the run, and rejections after that mean the probe stopped working
-// rather than the backend.
-func TestProber_RejectedProbeReadmitsADownBackendOnlyAfterASuccess(t *testing.T) {
+// Rejections cannot talk a down backend back into the pool while a real failure
+// is still inside the window — the failure is still the best evidence there is.
+// Once it has aged out and nothing but refusals is left, the probe rather than
+// the backend is what is broken.
+func TestProber_RejectionsReadmitADownBackendOnlyAfterTheFailureAgesOut(t *testing.T) {
 	p := newTestProber(t, healthConfig("http://a.invalid", "http://b.invalid"))
 	markDown(t, p, "a")

-	for i := 0; i < p.failures*5; i++ {
+	for i := 1; i < p.window; i++ {
 		p.record("a", rejectedProbe(p))
 		if p.healthy("a") {
-			t.Fatal("rejections re-admitted a backend whose failure run never ended")
+			t.Fatalf("rejection %d of a %d-probe window re-admitted a backend whose failure is still in view", i, p.window)
+		}
+		if got := p.snapshot()["a"].stateName(); got != stateUnhealthy {
+			t.Fatalf("state = %q after rejection %d, want %q", got, i, stateUnhealthy)
 		}
 	}
-	if got := p.snapshot()["a"].stateName(); got != stateUnhealthy {
-		t.Fatalf("state = %q, want %q", got, stateUnhealthy)
+
+	p.record("a", rejectedProbe(p))
+	if !p.healthy("a") {
+		t.Fatalf("backend stayed excluded after its last failure left the %d-probe window", p.window)
+	}
+	if got := p.snapshot()["a"].stateName(); got != stateProbeUnsupported {
+		t.Fatalf("state = %q, want %q", got, stateProbeUnsupported)
 	}

 	p.record("a", nil)
 	markUnsupported(t, p, "a")
 }

+// A success clears the failure window as well as the run, so what follows it is
+// judged on its own evidence.
+func TestProber_SuccessClearsTheFailureWindow(t *testing.T) {
+	p := newTestProber(t, healthConfig("http://a.invalid", "http://b.invalid"))
+	markDown(t, p, "a")
+
+	for i := 0; i < p.successes; i++ {
+		p.record("a", nil)
+	}
+	if !p.healthy("a") {
+		t.Fatal("backend did not recover after its success threshold")
+	}
+	markUnsupported(t, p, "a")
+}
+
+// The stranding regression: a backend whose probe path is permanently wrong can
+// never answer a probe successfully, so one transient failure in the middle of
+// its 404s must not exclude it for the life of the process.
+func TestProber_AgedOutFailureSettlesOnProbeUnsupported(t *testing.T) {
+	p := newTestProber(t, healthConfig("http://a.invalid", "http://b.invalid"))
+	for i := 0; i < p.failures*3; i++ {
+		p.record("a", rejectedProbe(p))
+	}
+	if got := p.snapshot()["a"].stateName(); got != stateProbeUnsupported {
+		t.Fatalf("state = %q before the failure, want %q", got, stateProbeUnsupported)
+	}
+
+	p.record("a", errors.New("read: connection reset by peer"))
+	if p.healthy("a") {
+		t.Fatal("a real failure did not take the backend out of the pool")
+	}
+
+	for i := 1; i < p.window; i++ {
+		p.record("a", rejectedProbe(p))
+		if p.healthy("a") {
+			t.Fatalf("the failure aged out after %d of %d probes", i, p.window)
+		}
+	}
+	p.record("a", rejectedProbe(p))
+
+	for i := 0; i < 500; i++ {
+		p.record("a", rejectedProbe(p))
+		if !p.healthy("a") {
+			t.Fatalf("backend was still excluded %d probes after its failure aged out", i+p.window)
+		}
+		if got := p.snapshot()["a"].stateName(); got != stateProbeUnsupported {
+			t.Fatalf("state = %q, want %q", got, stateProbeUnsupported)
+		}
+	}
+}
+
+// The window is the failure threshold, floored so it always spans a
+// reject-heavy cycle.
+func TestProber_FailureWindowFollowsTheThreshold(t *testing.T) {
+	for _, tc := range []struct{ failures, window int }{
+		{1, minFailureWindow}, {2, minFailureWindow}, {3, 3}, {5, 5}, {9, 9},
+	} {
+		cfg := healthConfig("http://a.invalid", "http://b.invalid")
+		cfg.HealthProbeFailures = tc.failures
+		p := newTestProber(t, cfg)
+		if p.window != tc.window {
+			t.Fatalf("health_probe_failures = %d gave a window of %d, want %d", tc.failures, p.window, tc.window)
+		}
+
+		markDown(t, p, "a")
+		for i := 1; i < tc.window; i++ {
+			p.record("a", rejectedProbe(p))
+			if p.healthy("a") {
+				t.Fatalf("threshold %d: failure aged out after %d of %d probes", tc.failures, i, tc.window)
+			}
+		}
+		p.record("a", rejectedProbe(p))
+		if !p.healthy("a") {
+			t.Fatalf("threshold %d: failure never aged out of the %d-probe window", tc.failures, tc.window)
+		}
+	}
+}
+
+// Every cycle that keeps failing, whatever it mixes in, must settle down and stay
+// down: once a backend has left the pool nothing but a success may bring it back.
+func TestProber_MixedFailureCyclesNeverFlapBackIntoService(t *testing.T) {
+	rejected := func(p *prober) error { return rejectedProbe(p) }
+	failed := func(*prober) error { return errors.New("boom") }
+
+	cycles := map[string][]func(*prober) error{
+		"rejected/failure":      {rejected, failed},
+		"failure/rejected":      {failed, rejected},
+		"reject-heavy 3-cycle":  {rejected, rejected, failed},
+		"failure-heavy 3-cycle": {rejected, failed, failed},
+		"failure-led 3-cycle":   {failed, rejected, rejected},
+		"paired 4-cycle":        {rejected, rejected, failed, failed},
+	}
+	for name, cycle := range cycles {
+		for _, failures := range []int{1, 2, 3, 5} {
+			t.Run(fmt.Sprintf("%s/threshold %d", name, failures), func(t *testing.T) {
+				cfg := healthConfig("http://a.invalid", "http://b.invalid")
+				cfg.HealthProbeFailures = failures
+				p := newTestProber(t, cfg)
+
+				down := false
+				for i := 0; i < 600; i++ {
+					p.record("a", cycle[i%len(cycle)](p))
+					switch {
+					case !p.healthy("a"):
+						down = true
+					case down:
+						t.Fatalf("backend flapped back into service at probe %d", i+1)
+					}
+				}
+				if !down {
+					t.Fatal("a backend that failed 600 probes never left the pool")
+				}
+				if got := p.snapshot()["a"].stateName(); got != stateUnhealthy {
+					t.Errorf("state = %q, want %q", got, stateUnhealthy)
+				}
+			})
+		}
+	}
+}
+
 func TestProber_LogsProbeRejectionOncePerTransition(t *testing.T) {
 	var buf strings.Builder
 	p := newProber(healthConfig("http://a.invalid", "http://b.invalid"), log.New(&buf, "", 0))