From 2c46c6c2dbda89ea8e3a93194a8de965aed9e2b6 Mon Sep 17 00:00:00 2001
From: rcourtman <courtmanr@gmail.com>
Date: Thu, 28 May 2026 13:12:55 +0100
Subject: [PATCH] Stop swallowing alert config persistence errors

Refs #1341

UpdateAlertConfig used to log SaveAlertConfig failures and still tell
the client "saved successfully", leaving the in-memory state with the
new override but the on-disk file untouched. On the next config reload
or process restart, the override silently vanished and the user saw
their threshold "revert" with no surfaced error. Return HTTP 500 with
the persistence error so the frontend can show a real save-failed
toast instead of false confidence.
---
 internal/api/alerts.go      | 10 ++++++++--
 internal/api/alerts_test.go | 38 +++++++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+), 2 deletions(-)

diff --git a/internal/api/alerts.go b/internal/api/alerts.go
index cc33613ca..f2d64ba48 100644
--- a/internal/api/alerts.go
+++ b/internal/api/alerts.go
@@ -3,6 +3,7 @@ package api
 import (
 	"context"
 	"encoding/json"
+	"fmt"
 	"io"
 	"net/http"
 	"net/url"
@@ -199,10 +200,15 @@ func (h *AlertHandlers) UpdateAlertConfig(w http.ResponseWriter, r *http.Request
 	)
 	notificationMgr.SetNotifyOnResolve(updatedConfig.Schedule.NotifyOnResolve)
 
-	// Save to persistent storage
+	// Save to persistent storage. Failure here used to be swallowed (logged
+	// and reported "success"), which led to the in-memory state diverging
+	// from what was actually persisted. On the next restart or config
+	// reload, the override silently vanished. Surface the error so the
+	// client can show a real save-failed signal.
 	if err := h.getMonitor(r.Context()).GetConfigPersistence().SaveAlertConfig(updatedConfig); err != nil {
-		// Log error but don't fail the request
 		log.Error().Err(err).Msg("Failed to save alert configuration")
+		http.Error(w, fmt.Sprintf("Failed to save alert configuration: %v", err), http.StatusInternalServerError)
+		return
 	}
 
 	if err := utils.WriteJSONResponse(w, map[string]interface{}{
diff --git a/internal/api/alerts_test.go b/internal/api/alerts_test.go
index ed2fb4ee9..d786099d5 100644
--- a/internal/api/alerts_test.go
+++ b/internal/api/alerts_test.go
@@ -5,6 +5,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"net/http"
 	"net/http/httptest"
 	"strings"
 	"testing"
@@ -167,6 +168,43 @@ func TestUpdateAlertConfig(t *testing.T) {
 	assert.False(t, notificationMgr.IsEnabled())
 }
 
+// Regression for #1341: persistence failures used to be silently swallowed
+// (logged but reported as "saved successfully"), so a user setting a 50%
+// per-pool override would see the value stick in memory and revert on
+// reload, with no clue why. Surface the error so the frontend can show a
+// real save-failed signal.
+func TestUpdateAlertConfig_PersistenceFailureSurfacesAsError(t *testing.T) {
+	mockMonitor := new(MockAlertMonitor)
+	mockManager := new(MockAlertManager)
+	mockPersist := new(MockConfigPersistence)
+	notificationMgr := notifications.NewNotificationManager("")
+	defer notificationMgr.Stop()
+
+	mockMonitor.On("GetAlertManager").Return(mockManager)
+	mockMonitor.On("GetConfigPersistence").Return(mockPersist)
+	mockMonitor.On("GetNotificationManager").Return(notificationMgr)
+
+	h := NewAlertHandlers(nil, mockMonitor, nil)
+
+	cfg := alerts.AlertConfig{Enabled: true, ActivationState: alerts.ActivationPending}
+	mockManager.On("UpdateConfig", testifymock.Anything).Return()
+	mockManager.On("GetConfig").Return(cfg)
+	mockPersist.On("SaveAlertConfig", testifymock.Anything).Return(errors.New("permission denied"))
+
+	body, _ := json.Marshal(cfg)
+	req := httptest.NewRequest("POST", "/api/alerts/config", bytes.NewReader(body))
+	w := httptest.NewRecorder()
+
+	h.UpdateAlertConfig(w, req)
+
+	if w.Code != http.StatusInternalServerError {
+		t.Fatalf("status = %d, want 500 on persistence failure", w.Code)
+	}
+	if !strings.Contains(w.Body.String(), "permission denied") {
+		t.Fatalf("response body should expose persistence error, got %q", w.Body.String())
+	}
+}
+
 func TestActivateAlerts_EnablesNotificationManager(t *testing.T) {
 	mockMonitor := new(MockAlertMonitor)
 	mockManager := new(MockAlertManager)