From bd7d196c110970ff423805e1381bd212483d04ef Mon Sep 17 00:00:00 2001 From: rcourtman Date: Fri, 8 May 2026 19:53:03 +0100 Subject: [PATCH] Mark failed PBS poll as failure and lock down with regression tests The PBS poller's version-failure exit at monitor_pbs_pmg.go did not set pollErr before returning, so even after fixing the defer-arg capture in bf6261adc the deferred recordTaskResult still saw nil and recorded the poll as a success. PMG's analogous path already sets pollErr correctly. Mirror that here so the per-instance pollStatusMap, the connections aggregator, and the circuit breaker all see PBS auth and version failures as failures. Add assertions to the existing PBS and PMG auth-failure tests that the per-instance pollStatusMap entry has a zero LastSuccess and a non-zero ConsecutiveFailures. The original tests covered downstream state but not the recorder, which is why two distinct cases of this class of bug went unnoticed. --- internal/monitoring/monitor_pbs_coverage_test.go | 15 +++++++++++++++ internal/monitoring/monitor_pbs_pmg.go | 1 + internal/monitoring/monitor_pmg_test.go | 15 +++++++++++++++ 3 files changed, 31 insertions(+) diff --git a/internal/monitoring/monitor_pbs_coverage_test.go b/internal/monitoring/monitor_pbs_coverage_test.go index bff95404e..d5a6a5c1b 100644 --- a/internal/monitoring/monitor_pbs_coverage_test.go +++ b/internal/monitoring/monitor_pbs_coverage_test.go @@ -64,6 +64,21 @@ func TestMonitor_PollPBSInstance_AuthFailure(t *testing.T) { t.Error("Expected connection health to be false") } + // Regression: pollStatusMap must record the failure. A defer-arg bug + // previously captured pollErr at register-time (always nil), so failed + // polls were recorded as success and the connections aggregator reported + // broken instances as healthy. + if status := m.pollStatusMap["pbs::pbs-auth-fail"]; status == nil { + t.Fatal("expected pollStatusMap entry for pbs::pbs-auth-fail, got nil") + } else { + if !status.LastSuccess.IsZero() { + t.Errorf("expected LastSuccess to remain zero on auth failure, got %v", status.LastSuccess) + } + if status.ConsecutiveFailures == 0 { + t.Error("expected ConsecutiveFailures > 0 after auth failure, got 0") + } + } + // We can't easily check authFailures map as it is private and no getter (except checking if it backs off?) } diff --git a/internal/monitoring/monitor_pbs_pmg.go b/internal/monitoring/monitor_pbs_pmg.go index 0418b222c..b16958bc2 100644 --- a/internal/monitoring/monitor_pbs_pmg.go +++ b/internal/monitoring/monitor_pbs_pmg.go @@ -188,6 +188,7 @@ func (m *Monitor) pollPBSInstance(ctx context.Context, instanceName string, clie pbsInst.Status = "offline" pbsInst.ConnectionHealth = "error" monErr := errors.WrapConnectionError("get_pbs_version", instanceName, versionErr) + pollErr = monErr log.Error().Err(monErr).Str("instance", instanceName).Msg("failed to connect to PBS") m.setProviderConnectionHealth(InstanceTypePBS, instanceName, false) diff --git a/internal/monitoring/monitor_pmg_test.go b/internal/monitoring/monitor_pmg_test.go index a8647b187..43aa45060 100644 --- a/internal/monitoring/monitor_pmg_test.go +++ b/internal/monitoring/monitor_pmg_test.go @@ -249,4 +249,19 @@ func TestPollPMGInstanceRecordsAuthFailures(t *testing.T) { if failures := mon.authFailures["pmg-failing"]; failures != 1 { t.Fatalf("expected one auth failure tracked, got %d", failures) } + + // Regression: pollStatusMap must record the failure. A defer-arg bug + // previously captured pollErr at register-time (always nil), so failed + // polls were recorded as success and the connections aggregator reported + // broken instances as healthy. + status := mon.pollStatusMap["pmg::failing"] + if status == nil { + t.Fatal("expected pollStatusMap entry for pmg::failing, got nil") + } + if !status.LastSuccess.IsZero() { + t.Errorf("expected LastSuccess to remain zero on auth failure, got %v", status.LastSuccess) + } + if status.ConsecutiveFailures == 0 { + t.Error("expected ConsecutiveFailures > 0 after auth failure, got 0") + } }