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)