From 7fd5f72de1ccf024f06db47ac53f311411061ad8 Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Sun, 6 Sep 2026 00:28:01 +1000 Subject: [PATCH] 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. --- README.md | 19 +++++-- health.go | 38 +++++++++---- health_test.go | 147 ++++++++++++++++++++++++++++++++++++++++++++++--- 3 files changed, 178 insertions(+), 26 deletions(-) diff --git a/README.md b/README.md index 344f012..c7981a0 100644 --- a/README.md +++ b/README.md @@ -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; diff --git a/health.go b/health.go index 23d9b47..7ff2822 100644 --- a/health.go +++ b/health.go @@ -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 { diff --git a/health_test.go b/health_test.go index 4a3e040..a030420 100644 --- a/health_test.go +++ b/health_test.go @@ -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))