Count one run of failed probes, whatever kind they are

Resetting the run on every change of outcome kind meant only consecutive
same-kind probes accumulated, so a backend failing every probe but
alternating kinds — a fronting proxy flipping 404 and 503 — never reached
the failure threshold and kept taking every query.

- count one run of consecutive not-OK probes for the down decision, and use
  the kinds only to pick which non-healthy state the run enters
- send a run containing any real failure to unhealthy; only a run of nothing
  but refusals enters probe_unsupported and stays in service
- classify 429 as a real failure: it is the backend reporting its own
  capacity, so an overloaded backend gets backed off
- document that probe_unsupported means "not verified" and that reachable is
  the /healthz field carrying actual reachability

README.md

@@ -230,26 +230,32 @@ not answering.
   still works.
 - **A refused probe is not a sick backend.** Only evidence about the *backend*
   takes it out of the pool: a transport failure (connection refused, DNS, TLS,
-  timeout) or a `5xx` — `503` included, since trapperkeeper answers `503` exactly
-  when its services are not nominal, so it stays a real failure. A reply that
-  refuses the *probe request* is evidence about the probe instead: every `4xx` is
-  the backend answering that our request is the problem (`404`/`410` the path is
-  not there, `405` it does not take a `GET`, `401`/`403` we are not allowed to
-  ask, `429` we asked too often), and `501` says it does not implement the
+  timeout), a `5xx` — `503` included, since trapperkeeper answers `503` exactly
+  when its services are not nominal — or a `429`, which is the backend reporting
+  its own capacity rather than judging the request, so an overloaded backend gets
+  backed off instead of kept at full traffic. A reply that refuses the *probe
+  request* is evidence about the probe instead: the other `4xx` are the backend
+  answering that our request is the problem (`404`/`410` the path is not there,
+  `405` it does not take a `GET`, `401`/`403` we are not allowed to ask), and
+  `501` says it does not implement the
   endpoint. A backend answering that way is **left in service** — still queried,
   still contributing records — and reported as `probe_unsupported` rather than
   `healthy`, so an operator can tell "verified healthy" from "not actually being
   checked". A probe path that is wrong for one backend alone can therefore never
   strand a working backend. The misconfiguration is logged once per transition,
   naming the backend, the probe path and the status.
-- **Thresholds** — a healthy backend goes down after `health_probe_failures`
-  (default 3) **consecutive** failures; a down one comes back after
-  `health_probe_successes` (default 2) consecutive successes, and the same
-  thresholds gate `probe_unsupported` in and out. One blip cannot flap a backend
-  out, and one lucky reply cannot flap it back in. A run only counts while the
-  replies keep their kind, so a stretch of `404`s and a stretch of `503`s never
-  add up to one threshold. A down backend keeps being probed, so recovery is
-  automatic.
+- **Thresholds** — a healthy backend leaves the pool after
+  `health_probe_failures` (default 3) **consecutive** unsuccessful probes; a down
+  one comes back after `health_probe_successes` (default 2) consecutive
+  successes, and the same thresholds gate `probe_unsupported` in and out. One
+  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.
 - **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;
@@ -269,9 +275,14 @@ not answering.
   `consecutive_failures`,
   `consecutive_successes`, `last_probe` and `last_error`, alongside the
   `reachable` check `/healthz` runs itself — which always asks **every**
-  backend, so a backend queries are skipping is still reported. A `query` object
-  reports the last merged fan-out: `partial`, `contributed`, `configured`,
-  `partial_rounds` and `last_partial`.
+  backend, so a backend queries are skipping is still reported. Read the two
+  together: `state` is the prober's verdict and `probe_unsupported` means "not
+  being verified", *not* "well", so `reachable` is the field that says whether
+  the backend is answering right now. A proxy that `404`s everything because the
+  backend behind it is dead shows `state: probe_unsupported` with `reachable`
+  carrying the query error, and the overall `status` drops to `degraded` or
+  `down` accordingly. A `query` object reports the last merged fan-out:
+  `partial`, `contributed`, `configured`, `partial_rounds` and `last_partial`.
 - **`health_probe_enabled: false`** turns the whole thing off: no probing
   goroutines, no backend ever skipped, every backend queried on every request.
   `X-Backends` still reports how many answered.

health.go

@@ -42,11 +42,13 @@ type probeState struct {
 	healthy     bool
 	unsupported bool
 	probed      bool
-	outcome     probeOutcome
-	failures    int
-	successes   int
-	lastProbe   time.Time
-	lastErr     string
+	// 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
 }
 
 // backendHealth is a copy of one backend's probe state, safe to read outside the
@@ -86,11 +88,16 @@ func (e *probeRejectedError) Error() string {
 }
 
 // probeRejects reports whether a status code refuses the probe request rather
-// than reporting the service unhealthy. Every 4xx is the backend answering that
-// our request is the problem, and 501 says it does not implement the endpoint.
-// Other 5xx stay real evidence: trapperkeeper answers 503 exactly when its
-// services are not nominal.
+// than reporting the service unhealthy. A 4xx is the backend answering that our
+// request is the problem, and 501 says it does not implement the endpoint.
+// 429 is excluded: it is the backend reporting its own capacity, not a verdict
+// on the request, so an overloaded backend must get backed off rather than kept
+// at full traffic. Other 5xx stay real evidence too: trapperkeeper answers 503
+// exactly when its services are not nominal.
 func probeRejects(code int) bool {
+	if code == http.StatusTooManyRequests {
+		return false
+	}
 	return (code >= 400 && code < 500) || code == http.StatusNotImplemented
 }
 
@@ -235,7 +242,9 @@ func (p *prober) probeOne(ctx context.Context, b Backend) {
 
 // record applies one probe outcome, flipping state only once the matching
 // threshold is reached so a single blip cannot flap a backend in either
-// direction.
+// 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.
 func (p *prober) record(name string, err error) {
 	p.mu.Lock()
 	st, ok := p.states[name]
@@ -244,19 +253,13 @@ func (p *prober) record(name string, err error) {
 		return
 	}
 	outcome := classifyProbe(err)
-	// A run only counts while the outcome keeps its kind, so a rejected-probe run
-	// and a real-failure run never add up to one threshold.
-	if outcome != st.outcome {
-		st.failures, st.successes = 0, 0
-	}
-	st.outcome = outcome
 	st.probed = true
 	st.lastProbe = p.now()
 
 	var transition string
-	switch outcome {
-	case outcomeOK:
+	if outcome == outcomeOK {
 		st.lastErr = ""
+		st.failures, st.runFailed = 0, false
 		st.successes++
 		if st.successes >= p.successes {
 			switch {
@@ -268,23 +271,26 @@ func (p *prober) record(name string, err error) {
 				transition = fmt.Sprintf("info: backend %q is up after %d consecutive probe successes", name, st.successes)
 			}
 		}
-	case outcomeRejected:
-		st.lastErr = err.Error()
-		st.failures++
-		if !st.unsupported && st.failures >= p.failures {
-			// The backend answered and refused the probe request itself, so this says
-			// nothing about its health: it keeps serving queries, unverified.
-			st.unsupported, st.healthy = true, true
-			transition = fmt.Sprintf("warning: backend %q rejected the health probe %d times: %v; leaving it in service but unverified, fix health_probe_path or its authorization", name, st.failures, err)
-		}
-	default:
+	} else {
 		st.lastErr = err.Error()
+		st.successes = 0
 		st.failures++
+		st.runFailed = st.runFailed || outcome == outcomeFailure
 		if st.failures >= p.failures {
-			st.unsupported = false
-			if st.healthy {
-				st.healthy = false
-				transition = fmt.Sprintf("warning: backend %q is down after %d consecutive probe failures: %v", name, st.failures, err)
+			switch {
+			case st.runFailed:
+				// Some probe in this run was evidence about the backend itself, so the
+				// run is a failure run whatever else it also contains.
+				st.unsupported = false
+				if st.healthy {
+					st.healthy = false
+					transition = fmt.Sprintf("warning: backend %q is down after %d consecutive failed probes: %v", name, st.failures, err)
+				}
+			case !st.unsupported:
+				// Nothing but refusals of the probe request, which says nothing about the
+				// backend's health: it keeps serving queries, unverified.
+				st.unsupported, st.healthy = true, true
+				transition = fmt.Sprintf("warning: backend %q rejected the health probe %d times: %v; leaving it in service but unverified, fix health_probe_path or its authorization", name, st.failures, err)
 			}
 		}
 	}

health_test.go

@@ -86,10 +86,14 @@ func markDown(t *testing.T, p *prober, name string) {
 	}
 }
 
+func rejectedProbe(p *prober) error {
+	return &probeRejectedError{status: http.StatusNotFound, path: p.path}
+}
+
 func markUnsupported(t *testing.T, p *prober, name string) {
 	t.Helper()
 	for i := 0; i < p.failures; i++ {
-		p.record(name, &probeRejectedError{status: http.StatusNotFound, path: p.path})
+		p.record(name, rejectedProbe(p))
 	}
 	if !p.healthy(name) {
 		t.Fatalf("backend %q was excluded by a rejected probe", name)
@@ -507,7 +511,7 @@ func TestProber_RejectedProbeKeepsBackendInService(t *testing.T) {
 	codes := []int{
 		http.StatusBadRequest, http.StatusUnauthorized, http.StatusForbidden,
 		http.StatusNotFound, http.StatusMethodNotAllowed, http.StatusGone,
-		http.StatusTooManyRequests, http.StatusNotImplemented,
+		http.StatusNotImplemented,
 	}
 	for _, code := range codes {
 		sb := newStatusBackend(t)
@@ -537,7 +541,12 @@ func TestProber_RejectedProbeKeepsBackendInService(t *testing.T) {
 // 503 is trapperkeeper's answer when its services are not nominal, so it stays a
 // real failure; so does anything that never got an HTTP reply at all.
 func TestProber_ServerErrorsAndTransportFailuresStillMarkDown(t *testing.T) {
-	for _, code := range []int{http.StatusInternalServerError, http.StatusBadGateway, http.StatusServiceUnavailable, http.StatusGatewayTimeout} {
+	// 429 says the backend is out of capacity, which is evidence about the backend.
+	for _, code := range []int{
+		http.StatusInternalServerError, http.StatusBadGateway,
+		http.StatusServiceUnavailable, http.StatusGatewayTimeout,
+		http.StatusTooManyRequests,
+	} {
 		sb := newStatusBackend(t)
 		sb.reply(code, degradedStatusBody("error"))
 		p := newTestProber(t, healthConfig(sb.srv.URL, "http://b.invalid"))
@@ -573,29 +582,121 @@ func TestProber_ServerErrorsAndTransportFailuresStillMarkDown(t *testing.T) {
 	}
 }
 
-func TestProber_RejectedRunAndFailureRunDoNotAddUp(t *testing.T) {
+// A real failure landing in a rejected run outranks the rejections: the run is
+// one run of unsuccessful probes, and its worst evidence decides the state.
+func TestProber_RealFailureInARejectedRunMarksDown(t *testing.T) {
 	p := newTestProber(t, healthConfig("http://a.invalid", "http://b.invalid"))
 	markUnsupported(t, p, "a")
 	for i := 0; i < 20; i++ {
-		p.record("a", &probeRejectedError{status: http.StatusNotFound, path: p.path})
+		p.record("a", rejectedProbe(p))
 	}
 
-	// One real failure on top of a long rejected run must not trip the threshold.
 	p.record("a", errors.New("boom"))
-	if !p.healthy("a") {
-		t.Fatal("a single real failure after a rejected run marked the backend down")
-	}
-	for i := 1; i < p.failures; i++ {
-		p.record("a", errors.New("boom"))
-	}
 	if p.healthy("a") {
-		t.Fatal("a full run of real failures should mark the backend down")
+		t.Fatal("a real failure inside a rejected run left the backend in service")
 	}
 	if got := p.snapshot()["a"].stateName(); got != stateUnhealthy {
 		t.Errorf("state = %q, want %q", got, stateUnhealthy)
 	}
 }
 
+// The regression this guards: a backend failing every probe but flipping the
+// kind of failure — a fronting proxy alternating 404 and 503, say — never
+// answers a probe successfully, so it must go down at the threshold like any
+// other permanently failing backend.
+func TestProber_AlternatingOutcomeKindsStillMarkDown(t *testing.T) {
+	rejected := func(p *prober) error { return rejectedProbe(p) }
+	failed := func(*prober) error { return errors.New("boom") }
+	ok := func(*prober) error { return nil }
+
+	cycles := map[string][]func(*prober) error{
+		"rejected/failure":      {rejected, failed},
+		"failure/rejected":      {failed, rejected},
+		"failure-heavy 3-cycle": {rejected, failed, failed},
+		"reject-heavy 3-cycle":  {rejected, rejected, failed},
+	}
+	for name, cycle := range cycles {
+		t.Run(name, func(t *testing.T) {
+			p := newTestProber(t, healthConfig("http://a.invalid", "http://b.invalid"))
+			for i := 0; i < 200; i++ {
+				p.record("a", cycle[i%len(cycle)](p))
+			}
+			if p.healthy("a") {
+				t.Fatal("a backend that failed 200 probes is still in service")
+			}
+			if got := p.snapshot()["a"].stateName(); got != stateUnhealthy {
+				t.Errorf("state = %q, want %q", got, stateUnhealthy)
+			}
+		})
+	}
+
+	// The same kinds never trip while a success keeps ending the run.
+	p := newTestProber(t, healthConfig("http://a.invalid", "http://b.invalid"))
+	interleaved := []func(*prober) error{rejected, ok, failed, ok}
+	for i := 0; i < 200; i++ {
+		p.record("a", interleaved[i%len(interleaved)](p))
+		if !p.healthy("a") {
+			t.Fatal("a success between every unsuccessful probe still tripped the threshold")
+		}
+	}
+}
+
+// Reaching the threshold on nothing but refusals is the one case that stays in
+// service: it is evidence about the probe request, not about the backend.
+func TestProber_PurelyRejectedRunStaysInService(t *testing.T) {
+	p := newTestProber(t, healthConfig("http://a.invalid", "http://b.invalid"))
+	for i := 0; i < p.failures*10; i++ {
+		p.record("a", rejectedProbe(p))
+		if !p.healthy("a") {
+			t.Fatalf("a purely rejected run excluded the backend after %d probes", i+1)
+		}
+	}
+	if got := p.snapshot()["a"].stateName(); got != stateProbeUnsupported {
+		t.Fatalf("state = %q, want %q", got, stateProbeUnsupported)
+	}
+}
+
+// A single success resets the run whatever it was made of.
+func TestProber_SuccessResetsTheRunInEveryDirection(t *testing.T) {
+	for _, name := range []string{"rejected", "failure", "mixed"} {
+		t.Run(name, func(t *testing.T) {
+			p := newTestProber(t, healthConfig("http://a.invalid", "http://b.invalid"))
+			cfgFailures := p.failures
+			next := func(i int) error {
+				switch name {
+				case "rejected":
+					return rejectedProbe(p)
+				case "failure":
+					return errors.New("boom")
+				default:
+					if i%2 == 0 {
+						return rejectedProbe(p)
+					}
+					return errors.New("boom")
+				}
+			}
+			for i := 0; i < cfgFailures-1; i++ {
+				p.record("a", next(i))
+			}
+			p.record("a", nil)
+			snap := p.snapshot()["a"]
+			if snap.Failures != 0 {
+				t.Fatalf("consecutive_failures = %d after a success, want 0", snap.Failures)
+			}
+			// One short of the threshold again, so the pre-success run is gone.
+			for i := 0; i < cfgFailures-1; i++ {
+				p.record("a", next(i))
+			}
+			if !p.healthy("a") {
+				t.Fatal("the run survived a success")
+			}
+			if got := p.snapshot()["a"].stateName(); got == stateProbeUnsupported {
+				t.Fatal("a sub-threshold run reached probe_unsupported")
+			}
+		})
+	}
+}
+
 func TestProber_RecoversFromProbeUnsupported(t *testing.T) {
 	p := newTestProber(t, healthConfig("http://a.invalid", "http://b.invalid"))
 	markUnsupported(t, p, "a")
@@ -611,11 +712,25 @@ func TestProber_RecoversFromProbeUnsupported(t *testing.T) {
 	}
 }
 
-// A rejected probe re-admits a backend the prober had already marked down: the
-// evidence that condemned it has been replaced by no evidence at all.
-func TestProber_RejectedProbeReadmitsADownBackend(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) {
 	p := newTestProber(t, healthConfig("http://a.invalid", "http://b.invalid"))
 	markDown(t, p, "a")
+
+	for i := 0; i < p.failures*5; i++ {
+		p.record("a", rejectedProbe(p))
+		if p.healthy("a") {
+			t.Fatal("rejections re-admitted a backend whose failure run never ended")
+		}
+	}
+	if got := p.snapshot()["a"].stateName(); got != stateUnhealthy {
+		t.Fatalf("state = %q, want %q", got, stateUnhealthy)
+	}
+
+	p.record("a", nil)
 	markUnsupported(t, p, "a")
 }
 
@@ -710,6 +825,38 @@ func TestHealthz_ReportsProbeUnsupported(t *testing.T) {
 	}
 }
 
+// probe_unsupported alone cannot say whether a backend is alive: a proxy that
+// 404s everything because the backend behind it is dead looks exactly like a
+// misconfigured probe path. reachable is the field that separates them.
+func TestHealthz_ProbeUnsupportedSeparatesReachableFromDead(t *testing.T) {
+	a := newFakeBackend(t, `[]`, `[]`)
+	b := newFakeBackend(t, `[]`, `[]`)
+	srv := newTestServer(healthConfig(a.srv.URL, b.srv.URL))
+	markUnsupported(t, srv.health, "a")
+	markUnsupported(t, srv.health, "b")
+	b.fail = true
+
+	rec := doGet(t, srv.Handler(), "/healthz", "")
+	var hr healthReport
+	if err := json.Unmarshal(rec.Body.Bytes(), &hr); err != nil {
+		t.Fatal(err)
+	}
+	for name, got := range map[string]string{"a": hr.Backends["a"].State, "b": hr.Backends["b"].State} {
+		if got != stateProbeUnsupported {
+			t.Fatalf("%s state = %q, want %q", name, got, stateProbeUnsupported)
+		}
+	}
+	if hr.Backends["a"].Reachable != "ok" {
+		t.Errorf("a reachable = %q, want ok", hr.Backends["a"].Reachable)
+	}
+	if hr.Backends["b"].Reachable == "ok" {
+		t.Error("a dead backend behind an unusable probe path still reports reachable=ok")
+	}
+	if hr.Status != "degraded" {
+		t.Errorf("status = %q, want degraded", hr.Status)
+	}
+}
+
 func TestStatusBodyHealthy_IgnoresBodiesWithoutStates(t *testing.T) {
 	for _, body := range []string{`{}`, `[]`, `null`, `{"svc":{}}`, `not json`} {
 		if err := statusBodyHealthy([]byte(body)); err != nil {

server.go

@@ -673,10 +673,18 @@ type healthReport struct {
 }
 
 // backendReport pairs this request's own reachability check with the background
-// prober's running state for the same backend.
+// prober's running state for the same backend. The two answer different
+// questions and must be read together: probe_unsupported means "not being
+// verified", not "well", so reachable is the field that says whether the backend
+// is actually answering queries right now.
 type backendReport struct {
-	Reachable string `json:"reachable"` // "ok" | error text
-	State     string `json:"state"`     // healthy | unhealthy | probe_unsupported | unprobed | unmonitored
+	// Reachable is this request's own live query to the backend, run against
+	// every configured backend regardless of probe state: "ok" or the error text.
+	Reachable string `json:"reachable"`
+	// State is the background prober's verdict. probe_unsupported means the probe
+	// path is unusable on this backend, so its health is unknown — read Reachable
+	// to find out whether it is up.
+	State     string `json:"state"` // healthy | unhealthy | probe_unsupported | unprobed | unmonitored
 	Failures  int    `json:"consecutive_failures"`
 	Successes int    `json:"consecutive_successes"`
 	LastProbe string `json:"last_probe,omitempty"`