diff --git a/docs/release-control/v6/internal/subsystems/alerts.md b/docs/release-control/v6/internal/subsystems/alerts.md index 9ed270b84b..bc2a9294f4 100644 --- a/docs/release-control/v6/internal/subsystems/alerts.md +++ b/docs/release-control/v6/internal/subsystems/alerts.md @@ -525,7 +525,7 @@ active-state and intent-policy linkage while persisting through the stable A per-metric threshold is off whenever its trigger is `<= 0`. That boundary is engine truth (`internal/alerts/canonical_metric.go`, `internal/alerts/config_runtime.go`), not a display convention: `-1` is the -value Pulse writes, and `0` disables the metric just as completely because +value the editors stage, and `0` disables the metric just as completely because earlier builds advertised `0` as the disable value and those overrides are still on disk. Every threshold editor reads that same `<= 0` rule through `frontend-modern/src/components/Alerts/alertResourceTableModel.ts` rather than @@ -543,7 +543,28 @@ the metric's enabled default instead, and only clears the override when the inherited default is already enabled. Editors must also not manufacture an off state out of an empty input: a cleared box is mid-edit, and coercing it to `0` disabled the metric in the engine while the row still showed On. Disabling is -the toggle's job, and the toggle writes the canonical `-1`. +the toggle's job, and the toggle stages the canonical `-1`. + +The save path writes the backend's default contract, not the editors' sentinel. +Alert config normalization reads a negative trigger on a global default as +unset and restores the factory threshold, so `buildAlertsConfigurationPayload` +in `frontend-modern/src/features/alerts/alertsConfigurationModel.ts` writes +every Off global default as trigger `0`. Sending the staged `-1` through made +every Off default with a positive factory threshold come back On at that +threshold after save and reload; only guest and node CPU, memory and disk +escaped, because normalization leaves them alone. Overrides keep `-1`, which +the override path stores as written. Because `-1` and `0` are the same +setting, the Global Defaults "Custom" badge compares Off values as equal +rather than by raw number, so the factory `-1` I/O defaults do not read +Custom against a saved `0`. An unset factory value stays distinct from Off, +since unset can mean the default follows another setting. A row's +custom-threshold marker keeps comparing raw values: it drives the row's revert +control and override removal, so an explicit `-1` override must stay visible +against an Off default even though both alert the same way. +`frontend-modern/src/features/alerts/__tests__/alertsConfigurationModel.test.ts` +pins the payload for every section, and +`internal/alerts/config_defaults_off_test.go` pins that a zero default stays +off through `UpdateConfig` and raises no alert. External availability-probe reporting loss owns one canonical `external-probe-unavailable` alert per assigned agent, not one alert per target. @@ -2650,8 +2671,10 @@ Metric enablement is an explicit On/Off interaction in row, global-default, mobile, and bulk editors. Enabled numeric inputs accept positive trigger values only and user-facing copy must not expose the persisted disable sentinel. Internally, the canonical `<= 0` read rule remains intact for legacy data, while -new Off actions write `-1`, so existing configuration and the API contract -continue to round-trip without making `0` a second customer-facing disable path. +new Off actions stage `-1` (saved as trigger `0` on global defaults, which the +backend otherwise resets to factory), so existing configuration and the API +contract continue to round-trip without making `0` a second customer-facing +disable path. Within the Proxmox tab, render-heavy ownership now further routes through `frontend-modern/src/components/Alerts/ThresholdsTableProxmoxNodesSection.tsx`, `frontend-modern/src/components/Alerts/ThresholdsTableProxmoxPBSSection.tsx`, diff --git a/frontend-modern/browser-verification/9d9b2eb5c709c139.json b/frontend-modern/browser-verification/9d9b2eb5c709c139.json new file mode 100644 index 0000000000..bdd24494ee --- /dev/null +++ b/frontend-modern/browser-verification/9d9b2eb5c709c139.json @@ -0,0 +1,55 @@ +{ + "version": 1, + "base_sha": "bab38a25b92684c978c4273200765152040624ed", + "verified_at": "2026-10-07T15:05:37Z", + "result": "passed", + "changed_paths": [ + "frontend-modern/src/components/Alerts/alertResourceTableModel.ts", + "frontend-modern/src/features/alerts/alertsConfigurationModel.ts" + ], + "content_sha256": { + "frontend-modern/src/components/Alerts/alertResourceTableModel.ts": "9d2cbc37fe80492a67b54b106a1c413ea1cbca66b1cd30e1c100d1c90a408eea", + "frontend-modern/src/features/alerts/alertsConfigurationModel.ts": "3070cf7df162eefd5653e5a6eb0a0722e2ffa04b33b12a81ebb2f38a4035c62f" + }, + "routes": [ + "/alerts/thresholds/proxmox", + "/alerts/thresholds/systems", + "/alerts/thresholds/kubernetes", + "/alerts/thresholds/truenas", + "/alerts/thresholds/vsphere", + "/alerts/thresholds/docker" + ], + "viewports": [ + { + "width": 1280, + "height": 900 + }, + { + "width": 1024, + "height": 768 + }, + { + "width": 390, + "height": 844 + }, + { + "width": 375, + "height": 812 + } + ], + "states": [ + "Private mock stack (PULSE_MOCK_MODE: 3 PVE nodes, 3 agents, 1 Kubernetes cluster, TrueNAS, vSphere) on a backend built from bab38a25b9, Vite on this tree, alerts activated, fresh data dir per run, no injected data", + "Before (origin/main payload builder): Proxmox Global Defaults CPU and Temp switched Off and saved; the API stored nodeDefaults.cpu {-1, 0} and nodeDefaults.temperature {80, 75}; after reload Temp read On 80", + "Final content, 1280 wide: Machines CPU Off saved as agentDefaults.cpu {0, 0} and reads Off after reload with the Custom badge (factory is 80)", + "Final content, 390 wide: Kubernetes Clusters Memory Off saved as kubernetesDefaults.memory {0, 0} and reads Off after reload", + "Same payload change, earlier passes: Proxmox Temp Off saved {0, 0} and reads Off after reload (1280); TrueNAS Systems CPU and vSphere Hosts CPU Off saved {0, 0} and read Off (390); Machines CPU at 1% raised 8 agent CPU alerts, switching it Off resolved all 8, Proxmox Temp Off resolved the node temperature alert, nothing re-fired over two minutes, values stayed Off across a backend restart (1024 and 375)", + "Custom badge on a fresh install: no Proxmox, Kubernetes, TrueNAS or vSphere Global Defaults group reads Custom (on the payload-only build Kubernetes, TrueNAS Systems and vSphere Hosts/VMs did); after an unrelated save (Proxmox node CPU 81) only Virtualization Hosts reads Custom and VMs & Containers stays clean", + "Docker Containers still reads Custom on a fresh install because the backend's no-alerts.json fallback stores Docker CPU/memory/disk as {0, 0}; separate pre-existing defect, not touched here", + "No page errors in any Playwright run" + ], + "interactions": [ + "Playwright: Expand all, clicked a Global Defaults On/Off badge, Save Changes, read GET /api/alerts/config, reloaded and re-read badge states and the Custom badge (Machines CPU at 1280x900, Kubernetes Memory at 390x844)", + "Playwright: recorded Custom badges on Proxmox, Kubernetes, TrueNAS, vSphere and Docker on a fresh install, changed Proxmox node CPU to 81, saved, recorded them again", + "Browser pane and Playwright (earlier passes on the same payload change): Machines CPU 1% then Off with /api/alerts/active reads, backend restart, Proxmox Temp, TrueNAS and vSphere CPU Off at phone width" + ] +} diff --git a/frontend-modern/src/components/Alerts/__tests__/alertResourceTableModel.branchcov2.test.ts b/frontend-modern/src/components/Alerts/__tests__/alertResourceTableModel.branchcov2.test.ts index 7b755212e6..40dd64ab6a 100644 --- a/frontend-modern/src/components/Alerts/__tests__/alertResourceTableModel.branchcov2.test.ts +++ b/frontend-modern/src/components/Alerts/__tests__/alertResourceTableModel.branchcov2.test.ts @@ -116,6 +116,30 @@ describe('hasCustomAlertResourceGlobalDefaults', () => { ).toBe(true); }); + it('reads an Off default saved as 0 as the factory Off of -1', () => { + expect( + hasCustomAlertResourceGlobalDefaults( + { cpu: 80, diskRead: 0, networkIn: 0 }, + { cpu: 80, diskRead: -1, networkIn: -1 }, + ), + ).toBe(false); + expect(hasCustomAlertResourceGlobalDefaults({ diskRead: 100 }, { diskRead: -1 })).toBe(true); + }); + + it('keeps an Off default custom against an unset factory value', () => { + // An unset factory value follows another setting (TrueNAS disks follow Disk + // temperature by type), so Off is a choice that needs its reset control. + expect( + hasCustomAlertResourceGlobalDefaults({ temperature: 0 }, { temperature: undefined }), + ).toBe(true); + expect( + hasCustomAlertResourceGlobalDefaults({ temperature: -1 }, { temperature: undefined }), + ).toBe(true); + expect( + hasCustomAlertResourceGlobalDefaults({ temperature: undefined }, { temperature: undefined }), + ).toBe(false); + }); + it('returns false when a factory key is absent from globalDefaults (current is undefined)', () => { expect(hasCustomAlertResourceGlobalDefaults({ memory: 90 }, { cpu: 80, memory: 90 })).toBe( false, diff --git a/frontend-modern/src/components/Alerts/alertResourceTableModel.ts b/frontend-modern/src/components/Alerts/alertResourceTableModel.ts index 8721befa98..d989dbafbd 100644 --- a/frontend-modern/src/components/Alerts/alertResourceTableModel.ts +++ b/frontend-modern/src/components/Alerts/alertResourceTableModel.ts @@ -101,7 +101,7 @@ export function hasCustomAlertResourceGlobalDefaults( return Object.keys(factoryDefaults).some((key) => { const current = globalDefaults[key]; const factory = factoryDefaults[key]; - return current !== undefined && current !== factory; + return current !== undefined && !alertResourceThresholdsEquivalent(current, factory); }); } @@ -208,12 +208,26 @@ export const ALERT_RESOURCE_METRIC_OFF_VALUE = -1; * The alert engine disables a metric whenever its trigger is `<= 0` * (`internal/alerts/canonical_metric.go`), so every editor surface has to read * `0` as Off too: older builds advertised `0` as the disable value and those - * overrides are still on disk. New writes always use the canonical `-1`. + * overrides are still on disk. Editors always stage the canonical `-1`; the + * save payload writes an Off global default as `0`, because the backend reads + * a negative default as unset. */ export function isAlertResourceMetricOff(value: number | undefined | null): boolean { return typeof value === 'number' && Number.isFinite(value) && value <= 0; } +/** + * Whether two thresholds alert the same way. Every value `<= 0` is Off, and a + * global default saves Off as `0` while the editors and factory defaults use + * `-1`, so the two compare equal. + */ +export function alertResourceThresholdsEquivalent( + left: number | undefined, + right: number | undefined, +): boolean { + return left === right || (isAlertResourceMetricOff(left) && isAlertResourceMetricOff(right)); +} + /** * Value an override editor must stage when a metric is switched back on. * Clearing the override (`undefined`) only re-inherits the default, so it diff --git a/frontend-modern/src/features/alerts/__tests__/alertsConfigurationModel.test.ts b/frontend-modern/src/features/alerts/__tests__/alertsConfigurationModel.test.ts index 71bb7bac74..cbb7e71946 100644 --- a/frontend-modern/src/features/alerts/__tests__/alertsConfigurationModel.test.ts +++ b/frontend-modern/src/features/alerts/__tests__/alertsConfigurationModel.test.ts @@ -1,5 +1,6 @@ import { describe, expect, it } from 'vitest'; +import { ALERT_RESOURCE_METRIC_OFF_VALUE } from '@/components/Alerts/alertResourceTableModel'; import type { AlertConfig } from '@/types/alerts'; import { @@ -224,4 +225,94 @@ describe('alertsConfigurationModel', () => { expect(result.alertConfig).toBeUndefined(); expect(result.dockerValidationError).toBe(ALERT_DOCKER_GAP_VALIDATION_ERROR); }); + + // The backend reads a negative default trigger as unset and restores the + // factory threshold, so every Global Defaults Off must reach it as 0. + it('writes an Off global default as trigger 0 in every section and reads it back Off', () => { + const thresholdMetrics = { + guestDefaults: ['cpu', 'memory', 'disk', 'diskRead', 'diskWrite', 'networkIn', 'networkOut'], + nodeDefaults: ['cpu', 'memory', 'disk', 'temperature'], + agentDefaults: ['cpu', 'memory', 'disk', 'diskTemperature'], + pbsDefaults: ['cpu', 'memory'], + kubernetesDefaults: [ + 'cpu', + 'memory', + 'disk', + 'diskRead', + 'diskWrite', + 'networkIn', + 'networkOut', + ], + trueNASDefaults: [ + 'cpu', + 'memory', + 'disk', + 'usage', + 'temperature', + 'diskRead', + 'diskWrite', + 'networkIn', + 'networkOut', + ], + trueNASDiskDefaults: ['temperature'], + vmwareDefaults: [ + 'cpu', + 'memory', + 'disk', + 'usage', + 'diskRead', + 'diskWrite', + 'networkIn', + 'networkOut', + ], + dockerDefaults: ['cpu', 'memory', 'disk'], + } as const; + const payloadSection = { + guestDefaults: 'guestDefaults', + nodeDefaults: 'nodeDefaults', + agentDefaults: 'agentDefaults', + pbsDefaults: 'pbsDefaults', + kubernetesDefaults: 'kubernetesDefaults', + trueNASDefaults: 'truenasDefaults', + trueNASDiskDefaults: 'truenasDiskDefaults', + vmwareDefaults: 'vmwareDefaults', + dockerDefaults: 'dockerDefaults', + } as const; + + type Section = keyof typeof thresholdMetrics; + + const snapshot = createDefaultAlertsConfigurationSnapshot(); + for (const [section, metrics] of Object.entries(thresholdMetrics)) { + const defaults: Record = snapshot[section as Section]; + for (const metric of metrics) { + defaults[metric] = ALERT_RESOURCE_METRIC_OFF_VALUE; + } + } + snapshot.storageDefault = ALERT_RESOURCE_METRIC_OFF_VALUE; + + const { alertConfig } = buildAlertsConfigurationPayload({ + snapshot, + rawOverridesConfig: {}, + alertsActivationState: null, + alertsActivationConfig: null, + }); + expect(alertConfig).toBeTruthy(); + const payload = alertConfig as unknown as Record>; + for (const [section, metrics] of Object.entries(thresholdMetrics)) { + const written = payload[payloadSection[section as Section]]; + for (const metric of metrics) { + expect(written[metric], `${section}.${metric}`).toEqual({ trigger: 0, clear: 0 }); + } + } + expect(alertConfig?.storageDefault).toEqual({ trigger: 0, clear: 0 }); + + const reloaded = readAlertsConfigurationSnapshot(alertConfig as AlertConfig); + for (const [section, metrics] of Object.entries(thresholdMetrics)) { + const defaults: Record = reloaded[section as Section]; + for (const metric of metrics) { + expect(defaults[metric], `${section}.${metric}`).toBe(0); + } + } + expect(reloaded.storageDefault).toBe(0); + }); }); diff --git a/frontend-modern/src/features/alerts/alertsConfigurationModel.ts b/frontend-modern/src/features/alerts/alertsConfigurationModel.ts index dd99913281..d719c7db1a 100644 --- a/frontend-modern/src/features/alerts/alertsConfigurationModel.ts +++ b/frontend-modern/src/features/alerts/alertsConfigurationModel.ts @@ -174,8 +174,11 @@ const cloneBackupDefaults = (backupDefaults: BackupAlertConfig): BackupAlertConf ignoreVMIDs: [...(backupDefaults.ignoreVMIDs ?? [])], }); +// Off is written as 0. The editors stage -1 for Off, but the backend reads a +// negative default trigger as unset and restores the factory threshold, so a +// saved -1 came back On. const createHysteresisThreshold = (trigger: number | undefined, clearMargin = 5) => { - const normalized = typeof trigger === 'number' ? trigger : 0; + const normalized = typeof trigger === 'number' && trigger > 0 ? trigger : 0; return { trigger: normalized, clear: Math.max(0, normalized - clearMargin), diff --git a/frontend-modern/src/features/alerts/thresholds/__tests__/thresholdsResourceModel.branchcov0723pm.test.ts b/frontend-modern/src/features/alerts/thresholds/__tests__/thresholdsResourceModel.branchcov0723pm.test.ts index 97378ef594..b1e18cd961 100644 --- a/frontend-modern/src/features/alerts/thresholds/__tests__/thresholdsResourceModel.branchcov0723pm.test.ts +++ b/frontend-modern/src/features/alerts/thresholds/__tests__/thresholdsResourceModel.branchcov0723pm.test.ts @@ -48,6 +48,14 @@ describe('thresholdsResourceModel branch coverage 0723pm', () => { expect(hasThresholdDiff(override, { cpu: 80, memory: 70 })).toBe(false); }); + it('keeps an explicit Off override visible against an Off default saved as 0', () => { + // The marker drives the row's revert control and override removal, so a + // stored -1 must not hide behind an Off default: switching the default + // back on would otherwise leave this resource silently off. + const override = makeOverride('a', { cpu: -1 }); + expect(hasThresholdDiff(override, { cpu: 0 })).toBe(true); + }); + it('returns true when one threshold diverges while a sibling matches', () => { const override = makeOverride('a', { cpu: 80, memory: 90 }); expect(hasThresholdDiff(override, { cpu: 80, memory: 70 })).toBe(true); diff --git a/internal/alerts/config_defaults_off_test.go b/internal/alerts/config_defaults_off_test.go new file mode 100644 index 0000000000..37e0faad74 --- /dev/null +++ b/internal/alerts/config_defaults_off_test.go @@ -0,0 +1,133 @@ +package alerts + +import ( + "testing" + "time" + + "github.com/rcourtman/pulse-go-rewrite/internal/models" +) + +// globalDefaultThresholds lists every pointer threshold the thresholds page +// edits in a Global Defaults row. +func globalDefaultThresholds(config *AlertConfig) map[string]**HysteresisThreshold { + return map[string]**HysteresisThreshold{ + "guest.cpu": &config.GuestDefaults.CPU, + "guest.memory": &config.GuestDefaults.Memory, + "guest.disk": &config.GuestDefaults.Disk, + "guest.diskRead": &config.GuestDefaults.DiskRead, + "guest.diskWrite": &config.GuestDefaults.DiskWrite, + "guest.networkIn": &config.GuestDefaults.NetworkIn, + "guest.networkOut": &config.GuestDefaults.NetworkOut, + "node.cpu": &config.NodeDefaults.CPU, + "node.memory": &config.NodeDefaults.Memory, + "node.disk": &config.NodeDefaults.Disk, + "node.temperature": &config.NodeDefaults.Temperature, + "agent.cpu": &config.AgentDefaults.CPU, + "agent.memory": &config.AgentDefaults.Memory, + "agent.disk": &config.AgentDefaults.Disk, + "agent.diskTemperature": &config.AgentDefaults.DiskTemperature, + "pbs.cpu": &config.PBSDefaults.CPU, + "pbs.memory": &config.PBSDefaults.Memory, + "kubernetes.cpu": &config.KubernetesDefaults.CPU, + "kubernetes.memory": &config.KubernetesDefaults.Memory, + "kubernetes.disk": &config.KubernetesDefaults.Disk, + "kubernetes.diskRead": &config.KubernetesDefaults.DiskRead, + "kubernetes.diskWrite": &config.KubernetesDefaults.DiskWrite, + "kubernetes.networkIn": &config.KubernetesDefaults.NetworkIn, + "kubernetes.networkOut": &config.KubernetesDefaults.NetworkOut, + "truenas.cpu": &config.TrueNASDefaults.CPU, + "truenas.memory": &config.TrueNASDefaults.Memory, + "truenas.disk": &config.TrueNASDefaults.Disk, + "truenas.usage": &config.TrueNASDefaults.Usage, + "truenas.temperature": &config.TrueNASDefaults.Temperature, + "truenas.diskRead": &config.TrueNASDefaults.DiskRead, + "truenas.diskWrite": &config.TrueNASDefaults.DiskWrite, + "truenas.networkIn": &config.TrueNASDefaults.NetworkIn, + "truenas.networkOut": &config.TrueNASDefaults.NetworkOut, + "truenas.disk.temperature": &config.TrueNASDiskDefaults.Temperature, + "vmware.cpu": &config.VMwareDefaults.CPU, + "vmware.memory": &config.VMwareDefaults.Memory, + "vmware.disk": &config.VMwareDefaults.Disk, + "vmware.usage": &config.VMwareDefaults.Usage, + "vmware.diskRead": &config.VMwareDefaults.DiskRead, + "vmware.diskWrite": &config.VMwareDefaults.DiskWrite, + "vmware.networkIn": &config.VMwareDefaults.NetworkIn, + "vmware.networkOut": &config.VMwareDefaults.NetworkOut, + } +} + +// The thresholds page saves a Global Defaults Off as trigger 0. Normalization +// reads a negative trigger as unset and restores the factory threshold, so 0 +// is the value that has to survive it in every section. +func TestUpdateConfigKeepsZeroTriggerGlobalDefaultsOff(t *testing.T) { + m := newTestManager(t) + config := m.GetConfig() + for _, field := range globalDefaultThresholds(&config) { + *field = &HysteresisThreshold{Trigger: 0, Clear: 0} + } + config.DockerDefaults.CPU = HysteresisThreshold{Trigger: 0, Clear: 0} + config.DockerDefaults.Memory = HysteresisThreshold{Trigger: 0, Clear: 0} + config.DockerDefaults.Disk = HysteresisThreshold{Trigger: 0, Clear: 0} + config.StorageDefault = HysteresisThreshold{Trigger: 0, Clear: 0} + + m.UpdateConfig(config) + + got := m.GetConfig() + for name, field := range globalDefaultThresholds(&got) { + if *field == nil || **field != (HysteresisThreshold{Trigger: 0, Clear: 0}) { + t.Errorf("%s = %+v after UpdateConfig, want trigger 0 (off)", name, *field) + } + } + for name, threshold := range map[string]HysteresisThreshold{ + "docker.cpu": got.DockerDefaults.CPU, + "docker.memory": got.DockerDefaults.Memory, + "docker.disk": got.DockerDefaults.Disk, + "storage": got.StorageDefault, + } { + if threshold != (HysteresisThreshold{Trigger: 0, Clear: 0}) { + t.Errorf("%s = %+v after UpdateConfig, want trigger 0 (off)", name, threshold) + } + } +} + +func TestCheckHostRaisesNoMetricAlertForAgentDefaultsSavedOff(t *testing.T) { + m := newTestManager(t) + m.ClearActiveAlerts() + config := m.GetConfig() + config.AgentDefaults.CPU = &HysteresisThreshold{Trigger: 0, Clear: 0} + config.AgentDefaults.Memory = &HysteresisThreshold{Trigger: 0, Clear: 0} + config.AgentDefaults.Disk = &HysteresisThreshold{Trigger: 0, Clear: 0} + m.UpdateConfig(config) + // UpdateConfig restores the default delay; without one a breach fires on + // the first check, so a missing alert means the metric is off. + m.mu.Lock() + m.config.TimeThresholds = map[string]int{} + m.mu.Unlock() + + host := models.Host{ + ID: "host-off", + DisplayName: "Off Host", + Hostname: "host-off.example", + Platform: "linux", + CPUUsage: 99, + CPUCount: 8, + Memory: models.Memory{Usage: 99, Total: 16384, Used: 16220, Free: 164}, + Disks: []models.Disk{{Mountpoint: "/", Usage: 99, Total: 100, Used: 99, Free: 1}}, + Status: "online", + IntervalSeconds: 30, + LastSeen: time.Now(), + } + m.CheckHost(host) + + resourceID := hostResourceID(host.ID) + diskResourceID, _ := hostDiskResourceID(host, host.Disks[0]) + for _, alertID := range []string{ + canonicalMetricStateID(resourceID, "cpu"), + canonicalMetricStateID(resourceID, "memory"), + canonicalMetricStateID(diskResourceID, "disk"), + } { + if _, exists := testLookupActiveAlert(t, m, alertID); exists { + t.Errorf("alert %q fired although its agent default was saved off", alertID) + } + } +}