fix(pbs): classify backup cache failures by HTTP status

A gateway body quoting API error 403 must not discard cached backups. Use the client's typed response status before legacy text fallback; cover gateway failures and genuine terminal responses.

Change-source: pulse-maintainer

docs/release-control/v6/internal/subsystems/monitoring.md

@@ -17,6 +17,22 @@
 
 ## Purpose
 
+Direct PBS backup polling classifies typed API and authentication failures by
+the response status exposed by `pbs.HTTPStatus`, including wrapped errors.
+A 5xx gateway or server response remains transient even when its body quotes
+an upstream “API error 403” or “API error 404”; that text must not erase the
+last known backup inventory. Genuine 4xx responses remain terminal under the
+existing cache policy. The legacy untyped-error fallback is unchanged.
+Retaining cached inventory does not establish a successful poll or fresh
+backup evidence.
+
+Verification: `TestPollPBSBackups_PreservesCacheOnTransientDatastoreError` and
+`TestPollPBSBackups_DropsStaleCacheOnTerminalDatastoreError` in
+`internal/monitoring/monitor_backups_readstate_test.go` exercise actual HTTP
+fixtures for 500, 502 quoting 403, 503 quoting 404, and genuine 401/403/404.
+These tests prove cache retention/removal, not installed PBS wake, service
+restart, or notification receipt.
+
 TrueNAS native alert projection preserves the trimmed, uppercase provider level in ResourceIncident.NativeSeverity. INFO and NOTICE retain the same canonical monitor risk; consumers must not lose their distinct actionability when projecting provider evidence. Native CRITICAL, ALERT, and EMERGENCY all project to canonical critical severity; EMERGENCY must not be discarded as unknown or make a still-active condition appear recovered. WARNING remains warning, and INFO and NOTICE remain informational at this projection boundary.
 
 Verification: `TestIncidentProjectionPreservesNativeSeverity` in `internal/truenas/provider_pool_health_contract_test.go` covers all seven native levels and case/whitespace normalization. `TestTrueNASNativeSeverityDispatch` in `internal/alerts/truenas_native_dispatch_test.go` verifies downstream INFO suppression, NOTICE preservation, notification severity, duplicate-poll retention, and confirmed recovery callback identity. The TrueNAS lifecycle tests in `internal/alerts/unified_incidents_test.go` require repeated EMERGENCY evidence to interrupt recovery confirmation. These are fixture-based projection and manager checks, not appliance ingestion or external notification-provider receipt proof.

internal/monitoring/backup_guard.go

@@ -1,6 +1,10 @@
 package monitoring
 
-import "strings"
+import (
+	"strings"
+
+	"github.com/rcourtman/pulse-go-rewrite/pkg/pbs"
+)
 
 // shouldPreservePBSBackupsWithTerminal preserves stale PBS backups only when all
 // datastore fetches failed and at least one failure was non-terminal.
@@ -17,6 +21,10 @@ func shouldReuseCachedPBSBackups(err error) bool {
 	if err == nil {
 		return false
 	}
+	if status, ok := pbs.HTTPStatus(err); ok {
+		return status < 400 || status >= 500
+	}
+	// Retain compatibility with untyped errors from older callers.
 	if strings.Contains(strings.ToLower(err.Error()), "api error 4") {
 		return false
 	}

internal/monitoring/monitor_backups_readstate_test.go

@@ -995,3 +995,117 @@ func TestRetirePVEInstanceRuntimeClearsPBSGuestConfirmations(t *testing.T) {
 		}
 	}
 }
+
+func TestPollPBSBackups_DropsStaleCacheOnTerminalDatastoreError(t *testing.T) {
+	t.Parallel()
+
+	for _, status := range []int{http.StatusUnauthorized, http.StatusForbidden, http.StatusNotFound} {
+		t.Run(http.StatusText(status), func(t *testing.T) {
+			t.Parallel()
+			server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+				if strings.Contains(r.URL.Path, "/admin/datastore/archive/groups") {
+					http.Error(w, `{"errors":"datastore does not exist"}`, status)
+					return
+				}
+				http.NotFound(w, r)
+			}))
+			defer server.Close()
+
+			client, err := pbs.NewClient(pbs.ClientConfig{
+				Host:       server.URL,
+				TokenName:  "root@pam!token",
+				TokenValue: "secret",
+			})
+			if err != nil {
+				t.Fatalf("failed to create PBS client: %v", err)
+			}
+
+			m := &Monitor{state: models.NewState()}
+			m.state.UpdatePBSBackups("pbs1", []models.PBSBackup{
+				{
+					ID:         "pbs-pbs1-archive--vm-100-1700000000",
+					Instance:   "pbs1",
+					Datastore:  "archive",
+					Namespace:  "",
+					BackupType: "vm",
+					VMID:       "100",
+					BackupTime: time.Unix(1700000000, 0),
+				},
+			})
+
+			m.pollPBSBackups(context.Background(), "pbs1", client, []models.PBSDatastore{
+				{Name: "archive"},
+			})
+
+			snapshot := m.state.GetSnapshot()
+			for _, backup := range snapshot.PBSBackups {
+				if backup.Instance == "pbs1" {
+					t.Fatalf("expected stale backups to be removed after terminal error, found: %+v", backup)
+				}
+			}
+		})
+	}
+}
+
+func TestPollPBSBackups_PreservesCacheOnTransientDatastoreError(t *testing.T) {
+	t.Parallel()
+
+	for _, tc := range []struct {
+		name   string
+		status int
+		body   string
+	}{
+		{"server error", http.StatusInternalServerError, "temporary server issue"},
+		{"gateway quoting forbidden", http.StatusBadGateway, "upstream API error 403: permission denied"},
+		{"unavailable quoting missing datastore", http.StatusServiceUnavailable, "upstream API error 404: datastore does not exist"},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+			server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+				if strings.Contains(r.URL.Path, "/admin/datastore/archive/groups") {
+					http.Error(w, tc.body, tc.status)
+					return
+				}
+				http.NotFound(w, r)
+			}))
+			defer server.Close()
+
+			client, err := pbs.NewClient(pbs.ClientConfig{
+				Host:       server.URL,
+				TokenName:  "root@pam!token",
+				TokenValue: "secret",
+			})
+			if err != nil {
+				t.Fatalf("failed to create PBS client: %v", err)
+			}
+
+			m := &Monitor{state: models.NewState()}
+			original := models.PBSBackup{
+				ID:         "pbs-pbs1-archive--vm-100-1700000000",
+				Instance:   "pbs1",
+				Datastore:  "archive",
+				Namespace:  "",
+				BackupType: "vm",
+				VMID:       "100",
+				BackupTime: time.Unix(1700000000, 0),
+			}
+			m.state.UpdatePBSBackups("pbs1", []models.PBSBackup{original})
+
+			m.pollPBSBackups(context.Background(), "pbs1", client, []models.PBSDatastore{
+				{Name: "archive"},
+			})
+
+			snapshot := m.state.GetSnapshot()
+			var found bool
+			for _, backup := range snapshot.PBSBackups {
+				if backup.Instance == "pbs1" && backup.ID == original.ID {
+					found = true
+					break
+				}
+			}
+			if !found {
+				t.Fatal("expected cached backup to be preserved on transient error")
+			}
+		})
+	}
+}

internal/monitoring/pbs_backup_cache_terminal_test.go

@@ -1,109 +0,0 @@
-package monitoring
-
-import (
-	"context"
-	"net/http"
-	"net/http/httptest"
-	"strings"
-	"testing"
-	"time"
-
-	"github.com/rcourtman/pulse-go-rewrite/internal/models"
-	"github.com/rcourtman/pulse-go-rewrite/pkg/pbs"
-)
-
-func TestPollPBSBackups_DropsStaleCacheOnTerminalDatastoreError(t *testing.T) {
-	t.Parallel()
-
-	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		if strings.Contains(r.URL.Path, "/admin/datastore/archive/groups") {
-			http.Error(w, `{"errors":"datastore does not exist"}`, http.StatusNotFound)
-			return
-		}
-		http.NotFound(w, r)
-	}))
-	defer server.Close()
-
-	client, err := pbs.NewClient(pbs.ClientConfig{
-		Host:       server.URL,
-		TokenName:  "root@pam!token",
-		TokenValue: "secret",
-	})
-	if err != nil {
-		t.Fatalf("failed to create PBS client: %v", err)
-	}
-
-	m := &Monitor{state: models.NewState()}
-	m.state.UpdatePBSBackups("pbs1", []models.PBSBackup{
-		{
-			ID:         "pbs-pbs1-archive--vm-100-1700000000",
-			Instance:   "pbs1",
-			Datastore:  "archive",
-			Namespace:  "",
-			BackupType: "vm",
-			VMID:       "100",
-			BackupTime: time.Unix(1700000000, 0),
-		},
-	})
-
-	m.pollPBSBackups(context.Background(), "pbs1", client, []models.PBSDatastore{
-		{Name: "archive"},
-	})
-
-	snapshot := m.state.GetSnapshot()
-	for _, backup := range snapshot.PBSBackups {
-		if backup.Instance == "pbs1" {
-			t.Fatalf("expected stale backups to be removed after terminal error, found: %+v", backup)
-		}
-	}
-}
-
-func TestPollPBSBackups_PreservesCacheOnTransientDatastoreError(t *testing.T) {
-	t.Parallel()
-
-	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		if strings.Contains(r.URL.Path, "/admin/datastore/archive/groups") {
-			http.Error(w, `{"errors":"temporary server issue"}`, http.StatusInternalServerError)
-			return
-		}
-		http.NotFound(w, r)
-	}))
-	defer server.Close()
-
-	client, err := pbs.NewClient(pbs.ClientConfig{
-		Host:       server.URL,
-		TokenName:  "root@pam!token",
-		TokenValue: "secret",
-	})
-	if err != nil {
-		t.Fatalf("failed to create PBS client: %v", err)
-	}
-
-	m := &Monitor{state: models.NewState()}
-	original := models.PBSBackup{
-		ID:         "pbs-pbs1-archive--vm-100-1700000000",
-		Instance:   "pbs1",
-		Datastore:  "archive",
-		Namespace:  "",
-		BackupType: "vm",
-		VMID:       "100",
-		BackupTime: time.Unix(1700000000, 0),
-	}
-	m.state.UpdatePBSBackups("pbs1", []models.PBSBackup{original})
-
-	m.pollPBSBackups(context.Background(), "pbs1", client, []models.PBSDatastore{
-		{Name: "archive"},
-	})
-
-	snapshot := m.state.GetSnapshot()
-	var found bool
-	for _, backup := range snapshot.PBSBackups {
-		if backup.Instance == "pbs1" && backup.ID == original.ID {
-			found = true
-			break
-		}
-	}
-	if !found {
-		t.Fatal("expected cached backup to be preserved on transient error")
-	}
-}

pkg/pbs/client.go

@@ -331,7 +331,9 @@ func (e *apiHTTPError) Error() string {
 	return message
 }
 
-func pbsHTTPStatus(err error) (int, bool) {
+// HTTPStatus returns the response status from a PBS API or authentication error,
+// including wrapped errors. The response body is never used for classification.
+func HTTPStatus(err error) (int, bool) {
 	var apiErr *apiHTTPError
 	if errors.As(err, &apiErr) {
 		return apiErr.status, true
@@ -346,12 +348,12 @@ func pbsHTTPStatus(err error) (int, bool) {
 }
 
 func isPBSPermissionError(err error) bool {
-	status, ok := pbsHTTPStatus(err)
+	status, ok := HTTPStatus(err)
 	return ok && (status == http.StatusUnauthorized || status == http.StatusForbidden)
 }
 
 func isPBSNotFoundError(err error) bool {
-	status, ok := pbsHTTPStatus(err)
+	status, ok := HTTPStatus(err)
 	return ok && status == http.StatusNotFound
 }
 

pkg/pbs/client_http_test.go

@@ -580,7 +580,7 @@ func TestClient_GetNodeName_SuperuserPermissionFailureRetries(t *testing.T) {
 			if firstErr == nil {
 				t.Fatal("first GetNodeName: expected error")
 			}
-			if got, ok := pbsHTTPStatus(firstErr); !ok || got != status {
+			if got, ok := HTTPStatus(firstErr); !ok || got != status {
 				t.Fatalf("first GetNodeName status = (%d, %v), want (%d, true): %v", got, ok, status, firstErr)
 			}
 			name, err := client.GetNodeName(context.Background())
@@ -632,7 +632,7 @@ func TestClient_GetNodeName_TransientHTTPFailuresRetryAndRecover(t *testing.T) {
 			if firstErr == nil {
 				t.Fatal("first GetNodeName: expected error")
 			}
-			if got, ok := pbsHTTPStatus(firstErr); !ok || got != tc.status {
+			if got, ok := HTTPStatus(firstErr); !ok || got != tc.status {
 				t.Fatalf("first GetNodeName status = (%d, %v), want (%d, true): %v", got, ok, tc.status, firstErr)
 			}
 			name, err := client.GetNodeName(context.Background())
@@ -795,7 +795,7 @@ func TestClient_GetNodeName_ConcurrentTransientFailureIsSingleFlight(t *testing.
 		if err == nil {
 			t.Fatal("concurrent GetNodeName: expected transient error")
 		}
-		if got, ok := pbsHTTPStatus(err); !ok || got != http.StatusServiceUnavailable {
+		if got, ok := HTTPStatus(err); !ok || got != http.StatusServiceUnavailable {
 			t.Fatalf("concurrent GetNodeName status = (%d, %v), want (503, true): %v", got, ok, err)
 		}
 	}
@@ -856,7 +856,7 @@ func TestClient_GetNodeStatus_PermissionOutageRecovery(t *testing.T) {
 		if status != nil || err == nil {
 			t.Fatalf("outage %d = (%+v, %v), want nil status and error", code, status, err)
 		}
-		if got, ok := pbsHTTPStatus(err); !ok || got != code {
+		if got, ok := HTTPStatus(err); !ok || got != code {
 			t.Fatalf("outage status = (%d, %v), want %d", got, ok, code)
 		}
 	}