From 539eadfab20badf7cb763bf4b130e21b7e5b1616 Mon Sep 17 00:00:00 2001
From: rcourtman <8825017+rcourtman@users.noreply.github.com>
Date: Wed, 7 Oct 2026 17:36:34 +0100
Subject: [PATCH] Judge filesystem alerts by their own threshold on config save

A config save re-judges every active alert. Agent filesystem alerts
were re-judged by the host disk threshold, ignoring the filesystem's
own override and the per-type DiskFillByType threshold CheckHost uses.
Guest filesystem alerts were resolved by their "<guestID>-disk-<key>"
resource ID, which matches no guest-disk override and names no guest,
so the guest's own override was missed too. A filesystem override of
70 raised an alert at 80 under a default of 90; saving any alert
setting resolved it and sent a recovery, and the next poll raised it
again as a new alert.

CheckHost and CheckGuest now resolve a filesystem's threshold through
hostDiskUsageThresholdNoLock and guestDiskUsageThresholdNoLock, and
reevaluation calls the same helpers: resolveHostAlertThresholdsNoLock
for agent filesystem alerts, and a new resolveGuestAlertThresholdsNoLock
that resolves a guest filesystem alert's guest by the ID
parseGuestAlertIdentity reads from its resource ID. The helpers return
override and per-type thresholds as owned copies, so evaluation still
never normalizes the live config.

TestConfigSaveJudgesFilesystemAlertsByTheirOwnThreshold covers an
agent filesystem override, an NVMe fill threshold, a guest filesystem
override and a guest override. With host.go, guest.go and
config_runtime.go overlaid by their origin/main or #2696 copies, all
four fail: the unchanged save resolves the alert.

Stacked on #2696.
---
 .../v6/internal/subsystems/alerts.md          |  28 +++++
 internal/alerts/config_runtime.go             |   2 +-
 internal/alerts/guest.go                      |  56 +++++++--
 internal/alerts/host.go                       |  78 +++++++-----
 .../threshold_resolution_shared_test.go       | 117 ++++++++++++++++++
 5 files changed, 234 insertions(+), 47 deletions(-)

diff --git a/docs/release-control/v6/internal/subsystems/alerts.md b/docs/release-control/v6/internal/subsystems/alerts.md
index 6e4f87d46f..e55ec55063 100644
--- a/docs/release-control/v6/internal/subsystems/alerts.md
+++ b/docs/release-control/v6/internal/subsystems/alerts.md
@@ -3163,6 +3163,34 @@ in place, is gone. `internal/alerts/alerts_test.go`
 evaluation against `GetConfig()` and `UpdateConfig(GetConfig())` under
 `-race`.
 
+### Config-save reevaluation judges a filesystem alert by its own threshold
+
+`UpdateConfig` re-judges every active alert, and a filesystem usage alert's
+threshold resolves through the same per-filesystem helper its evaluator uses.
+`hostDiskUsageThresholdNoLock` resolves an agent filesystem
+(`agent:<host>/disk:<label>`): its own override, then the `DiskFillByType`
+threshold for a hardware type inferred from the device while the host's disk
+alerting is on, then the host's disk threshold. `guestDiskUsageThresholdNoLock`
+resolves a guest filesystem (`<guestID>-disk-<key>`): its `guest-disk:`
+override, then the guest's disk threshold. `CheckHost` and `CheckGuest` call
+them under the read lock, and `resolveHostAlertThresholdsNoLock` and
+`resolveGuestAlertThresholdsNoLock` call them for reevaluation. Both return an
+override or per-type threshold as an owned copy. A guest filesystem alert
+resolves its guest's threshold, override and identity-based custom rules by the
+canonical `instance:node:vmid` guest ID that `parseGuestAlertIdentity` reads
+from its resource ID. Custom rules that filter on live metrics still see no
+readings there, as for every reevaluated guest alert. Reevaluation used to
+judge agent filesystems by the host threshold, and guest filesystems without
+their `guest-disk:` override or the guest's own override, because the
+filesystem's resource ID names no guest. Any settings save resolved an alert a
+per-disk threshold of 70 raised at 80 under a default of 90, notifying
+recovery, and the next poll raised it again as a new alert.
+`internal/alerts/threshold_resolution_shared_test.go`
+`TestConfigSaveJudgesFilesystemAlertsByTheirOwnThreshold` covers an agent
+filesystem override, an NVMe fill threshold, a guest filesystem override and a
+guest override: an unchanged save must keep the alert, and a save lifting the
+trigger above the reading must resolve it.
+
 ### Versioned alert-intent policy
 
 The alerts runtime owns one versioned alert-intent document and its durable
diff --git a/internal/alerts/config_runtime.go b/internal/alerts/config_runtime.go
index 6b12269ca4..9bc520b3d6 100644
--- a/internal/alerts/config_runtime.go
+++ b/internal/alerts/config_runtime.go
@@ -545,7 +545,7 @@ func (m *Manager) reevaluateActiveAlertsLocked() {
 				continue
 			}
 
-			guestThresholds := m.getGuestThresholds(guestSnapshotFromAlert(alert, resourceID), resourceID)
+			guestThresholds := m.resolveGuestAlertThresholdsNoLock(alert, resourceID)
 			if guestThresholds.Disabled {
 				alertsToResolve = append(alertsToResolve, alertID)
 				continue
diff --git a/internal/alerts/guest.go b/internal/alerts/guest.go
index edeac41cb3..6a0c694529 100644
--- a/internal/alerts/guest.go
+++ b/internal/alerts/guest.go
@@ -327,21 +327,12 @@ func (m *Manager) CheckGuest(guest any, instanceName string) {
 			seenDiskResources[perDiskResourceID] = struct{}{}
 			message := fmt.Sprintf("%s disk (%s) at %.1f%%", guestType, label, disk.Usage)
 
-			effectiveDiskThreshold := thresholds.Disk
-			// Own the override's Disk before ensureHysteresisThreshold fills
-			// Clear: the returned copy still points into the live config.
 			m.mu.RLock()
-			diskOverride, hasDiskOverride := lookupGuestDiskOverride(m.config.Overrides, guest, guestID, keySource)
-			diskOverride.Disk = cloneThreshold(diskOverride.Disk)
+			effectiveDiskThreshold, diskDisabled := m.guestDiskUsageThresholdNoLock(guest, guestID, keySource, thresholds.Disk)
 			m.mu.RUnlock()
-			if hasDiskOverride {
-				if diskOverride.Disabled {
-					m.clearAlert(canonicalMetricStateID(perDiskResourceID, "disk"))
-					continue
-				}
-				if diskOverride.Disk != nil {
-					effectiveDiskThreshold = ensureHysteresisThreshold(diskOverride.Disk)
-				}
+			if diskDisabled {
+				m.clearAlert(canonicalMetricStateID(perDiskResourceID, "disk"))
+				continue
 			}
 			if snapshot.DiskUnavailable {
 				if effectiveDiskThreshold == nil || effectiveDiskThreshold.Trigger <= 0 {
@@ -426,6 +417,45 @@ func guestDiskIdentity(disk models.Disk, idx int) (label, keySource, sanitizedKe
 	return label, keySource, sanitizedKey
 }
 
+// guestDiskUsageThresholdNoLock resolves the usage threshold one guest
+// filesystem is judged by: the filesystem's own override, else the guest's disk
+// threshold. disabled reports an override that switches the filesystem's
+// alerts off, and then the threshold is nil. CheckGuest and config-save
+// reevaluation both resolve through it. An override threshold is returned as
+// an owned copy, so normalizing it never writes into the live config. Callers
+// must hold m.mu.
+func (m *Manager) guestDiskUsageThresholdNoLock(guest any, guestID, diskKey string, guestDisk *HysteresisThreshold) (threshold *HysteresisThreshold, disabled bool) {
+	if override, ok := lookupGuestDiskOverride(m.config.Overrides, guest, guestID, diskKey); ok {
+		if override.Disabled {
+			return nil, true
+		}
+		if override.Disk != nil {
+			return ensureHysteresisThreshold(cloneThreshold(override.Disk)), false
+		}
+	}
+	return guestDisk, false
+}
+
+// resolveGuestAlertThresholdsNoLock resolves thresholds for a persisted guest
+// alert. A filesystem alert ("<guestID>-disk-<key>") resolves its guest's
+// thresholds by the guest ID, and its Disk by the filesystem's own override, as
+// CheckGuest does. Resolved by the filesystem's resource ID, which names no
+// guest, it missed both overrides, and a config save resolved an alert the next
+// poll raised again. Callers must hold m.mu.
+func (m *Manager) resolveGuestAlertThresholdsNoLock(alert *Alert, resourceID string) ThresholdConfig {
+	resourceID = strings.TrimSpace(resourceID)
+	if identity, ok := parseGuestAlertIdentity(resourceID); ok {
+		if diskKey, isFilesystem := strings.CutPrefix(identity.resourceSuffix, "-disk-"); isFilesystem {
+			guestID := strings.TrimSuffix(resourceID, identity.resourceSuffix)
+			guest := guestSnapshotFromAlert(alert, guestID)
+			thresholds := m.getGuestThresholds(guest, guestID)
+			thresholds.Disk, _ = m.guestDiskUsageThresholdNoLock(guest, guestID, diskKey, thresholds.Disk)
+			return thresholds
+		}
+	}
+	return m.getGuestThresholds(guestSnapshotFromAlert(alert, resourceID), resourceID)
+}
+
 func guestIORateMetrics(snapshot guestSnapshot) (diskRead, diskWrite, networkIn, networkOut *UnifiedResourceMetric) {
 	validity := snapshot.IORateValidity.EffectiveForRates(
 		snapshot.DiskRead,
diff --git a/internal/alerts/host.go b/internal/alerts/host.go
index 8c3edf1d7a..db47b5bdf6 100644
--- a/internal/alerts/host.go
+++ b/internal/alerts/host.go
@@ -172,9 +172,50 @@ func (m *Manager) resolveHostAlertThresholdsNoLock(alert *Alert, resourceID stri
 			thresholds.DiskTemperature = m.lowestHostDiskTemperatureThresholdNoLock(thresholds.DiskTemperature)
 		}
 	}
+	// A filesystem alert is judged by the threshold CheckHost evaluates that
+	// filesystem against, so a config save does not resolve an alert its
+	// override or per-type fill threshold still holds. A disabled filesystem
+	// resolves with no threshold, as CheckHost clears it.
+	if alert != nil && alert.Type == "disk" && isHostFilesystemResourceID(resourceID) {
+		thresholds.Disk, _ = m.hostDiskUsageThresholdNoLock(strings.TrimSpace(resourceID), metadataStringValue(alert.Metadata, "device"), thresholds.Disk)
+	}
 	return thresholds
 }
 
+// isHostFilesystemResourceID reports whether resourceID names one agent
+// filesystem ("agent:<host>/disk:<label>") rather than the host itself.
+func isHostFilesystemResourceID(resourceID string) bool {
+	_, child, ok := strings.Cut(stripHostResourcePrefix(resourceID), "/")
+	return ok && strings.HasPrefix(child, "disk:")
+}
+
+// hostDiskUsageThresholdNoLock resolves the usage threshold one agent
+// filesystem is judged by: the filesystem's own override, then the fill
+// threshold for its hardware type while the host's disk alerting is on, then
+// the host's disk threshold. disabled reports an override that switches the
+// filesystem's alerts off, and then the threshold is nil. CheckHost and
+// config-save reevaluation both resolve through it. An override or per-type
+// threshold is returned as an owned copy, so normalizing it never writes into
+// the live config. Callers must hold m.mu.
+func (m *Manager) hostDiskUsageThresholdNoLock(diskResourceID, device string, hostDisk *HysteresisThreshold) (threshold *HysteresisThreshold, disabled bool) {
+	if override, ok := m.config.Overrides[diskResourceID]; ok {
+		if override.Disabled {
+			return nil, true
+		}
+		if override.Disk != nil {
+			return ensureHysteresisThreshold(cloneThreshold(override.Disk)), false
+		}
+	}
+	if hostDisk != nil && hostDisk.Trigger > 0 {
+		if hwType := inferDiskHardwareType(device); hwType != "" {
+			if byType, ok := m.config.DiskFillByType[hwType]; ok {
+				return &byType, false
+			}
+		}
+	}
+	return hostDisk, false
+}
+
 func sanitizeHostComponent(value string) string {
 	value = strings.TrimSpace(strings.ToLower(value))
 	if value == "" {
@@ -450,41 +491,12 @@ func (m *Manager) CheckHost(host models.Host) {
 		diskResourceID, diskName := hostDiskResourceID(host, disk)
 		seenDisks[diskResourceID] = struct{}{}
 
-		// Check for disk-specific override. The copy's Disk still points into
-		// the live config, so own it before ensureHysteresisThreshold fills Clear.
 		m.mu.RLock()
-		diskOverride, hasDiskOverride := m.config.Overrides[diskResourceID]
-		diskOverride.Disk = cloneThreshold(diskOverride.Disk)
+		effectiveDiskThreshold, diskDisabled := m.hostDiskUsageThresholdNoLock(diskResourceID, disk.Device, thresholds.Disk)
 		m.mu.RUnlock()
-
-		// Determine the effective disk threshold
-		var effectiveDiskThreshold *HysteresisThreshold
-		if hasDiskOverride {
-			// If disk is disabled via override, skip alerting
-			if diskOverride.Disabled {
-				m.clearAlert(canonicalMetricStateID(diskResourceID, "disk"))
-				continue
-			}
-			// Use disk-specific threshold if set
-			if diskOverride.Disk != nil {
-				effectiveDiskThreshold = ensureHysteresisThreshold(diskOverride.Disk)
-			}
-		}
-		// Per-type override: consult DiskFillByType if hardware type is inferable
-		// from the device path and no disk-specific override applied above.
-		if effectiveDiskThreshold == nil && thresholds.Disk != nil && thresholds.Disk.Trigger > 0 {
-			if hwType := inferDiskHardwareType(disk.Device); hwType != "" {
-				m.mu.RLock()
-				if th, ok := m.config.DiskFillByType[hwType]; ok {
-					t := th
-					effectiveDiskThreshold = &t
-				}
-				m.mu.RUnlock()
-			}
-		}
-		// Fall back to host-level threshold
-		if effectiveDiskThreshold == nil {
-			effectiveDiskThreshold = thresholds.Disk
+		if diskDisabled {
+			m.clearAlert(canonicalMetricStateID(diskResourceID, "disk"))
+			continue
 		}
 
 		// Skip if no threshold configured (nil)
diff --git a/internal/alerts/threshold_resolution_shared_test.go b/internal/alerts/threshold_resolution_shared_test.go
index cf040367e4..fac87ae429 100644
--- a/internal/alerts/threshold_resolution_shared_test.go
+++ b/internal/alerts/threshold_resolution_shared_test.go
@@ -1350,6 +1350,123 @@ func TestGuestFilesystemEvidenceSuppressesDuplicateAggregateAlert(t *testing.T)
 	}
 }
 
+// A config save re-judges every active alert. A filesystem alert must be judged
+// by the threshold its evaluator used for that filesystem, not the host or guest
+// default: saving any unrelated setting used to resolve an alert a per-disk
+// threshold of 70 raised at 80 under a default of 90, notifying recovery, and
+// the next poll raised it again as a new alert.
+func TestConfigSaveJudgesFilesystemAlertsByTheirOwnThreshold(t *testing.T) {
+	perDisk := func() *HysteresisThreshold { return &HysteresisThreshold{Trigger: 70, Clear: 65} }
+	abovePerDisk := func() *HysteresisThreshold { return &HysteresisThreshold{Trigger: 85, Clear: 80} }
+
+	agentHost := func(device string) models.Host {
+		return models.Host{
+			ID: "host-fs", Hostname: "host-fs", Platform: "linux", Status: "online",
+			IntervalSeconds: 30, LastSeen: time.Now(),
+			Disks: []models.Disk{{Mountpoint: "/data", Device: device, Usage: 80, Total: 100, Used: 80, Free: 20}},
+		}
+	}
+	guestID := BuildGuestKey("pve1", "node1", 101)
+	guestVM := models.VM{
+		ID: guestID, VMID: 101, Name: "db", Node: "node1", Instance: "pve1", Status: "running",
+		Disks: []models.Disk{{Mountpoint: "/data", Device: "/dev/vdb1", Usage: 80, Total: 100, Used: 80, Free: 20}},
+	}
+	guestAlertID := canonicalMetricStateID(guestID+"-disk-data-dev-vdb1", "disk")
+
+	type diskCase struct {
+		name      string
+		alertID   string
+		configure func(cfg *AlertConfig)
+		evaluate  func(m *Manager)
+		// raise lifts the threshold that judges this filesystem above its reading.
+		raise func(cfg *AlertConfig)
+	}
+
+	agentDiskID, _ := hostDiskResourceID(agentHost("/dev/sdb1"), agentHost("/dev/sdb1").Disks[0])
+	guestDiskOverrideID := guestDiskOverrideKey(guestVM, guestID, "/data-/dev/vdb1")
+	guestOverrideID := guestOverridePrimaryKey(guestVM, guestID)
+
+	cases := []diskCase{
+		{
+			name:    "agent filesystem override",
+			alertID: canonicalMetricStateID(agentDiskID, "disk"),
+			configure: func(cfg *AlertConfig) {
+				cfg.Overrides = map[string]ThresholdConfig{agentDiskID: {Disk: perDisk()}}
+			},
+			evaluate: func(m *Manager) { m.CheckHost(agentHost("/dev/sdb1")) },
+			raise: func(cfg *AlertConfig) {
+				cfg.Overrides = map[string]ThresholdConfig{agentDiskID: {Disk: abovePerDisk()}}
+			},
+		},
+		{
+			name:    "agent disk fill by hardware type",
+			alertID: canonicalMetricStateID(agentDiskID, "disk"),
+			configure: func(cfg *AlertConfig) {
+				cfg.DiskFillByType["nvme"] = *perDisk()
+			},
+			evaluate: func(m *Manager) { m.CheckHost(agentHost("/dev/nvme0n1p1")) },
+			raise: func(cfg *AlertConfig) {
+				cfg.DiskFillByType["nvme"] = *abovePerDisk()
+			},
+		},
+		{
+			name:    "guest filesystem override",
+			alertID: guestAlertID,
+			configure: func(cfg *AlertConfig) {
+				cfg.Overrides = map[string]ThresholdConfig{guestDiskOverrideID: {Disk: perDisk()}}
+			},
+			evaluate: func(m *Manager) { m.CheckGuest(guestVM, "pve1") },
+			raise: func(cfg *AlertConfig) {
+				cfg.Overrides = map[string]ThresholdConfig{guestDiskOverrideID: {Disk: abovePerDisk()}}
+			},
+		},
+		{
+			name:    "guest override on a filesystem alert",
+			alertID: guestAlertID,
+			configure: func(cfg *AlertConfig) {
+				cfg.Overrides = map[string]ThresholdConfig{guestOverrideID: {Disk: perDisk()}}
+			},
+			evaluate: func(m *Manager) { m.CheckGuest(guestVM, "pve1") },
+			raise: func(cfg *AlertConfig) {
+				cfg.Overrides = map[string]ThresholdConfig{guestOverrideID: {Disk: abovePerDisk()}}
+			},
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			m := newTestManager(t)
+			cfg := m.GetConfig()
+			cfg.Enabled = true
+			// UpdateConfig fills missing delays with 5s; zero fires at once.
+			cfg.TimeThresholds = map[string]int{"agent": 0, "guest": 0}
+			cfg.AgentDefaults.Disk = &HysteresisThreshold{Trigger: 90, Clear: 85}
+			cfg.GuestDefaults.Disk = &HysteresisThreshold{Trigger: 90, Clear: 85}
+			tc.configure(&cfg)
+			m.UpdateConfig(cfg)
+
+			tc.evaluate(m)
+			startedAt := testRequireActiveAlert(t, m, tc.alertID).StartTime
+
+			m.UpdateConfig(m.GetConfig())
+			if !testHasActiveAlert(t, m, tc.alertID) {
+				t.Fatalf("an unchanged config save resolved %q, which its own threshold of 70 still holds at 80", tc.alertID)
+			}
+			tc.evaluate(m)
+			if got := testRequireActiveAlert(t, m, tc.alertID).StartTime; !got.Equal(startedAt) {
+				t.Fatalf("alert %q was raised again after the save: started %v, want %v", tc.alertID, got, startedAt)
+			}
+
+			cfg = m.GetConfig()
+			tc.raise(&cfg)
+			m.UpdateConfig(cfg)
+			if testHasActiveAlert(t, m, tc.alertID) {
+				t.Fatalf("a save lifting %q's own trigger above its reading kept the alert", tc.alertID)
+			}
+		})
+	}
+}
+
 // Regression: checkMetric stores canonical-identity alerts under the
 // canonical state key, so hysteresis resolution must not remove only the
 // unregistered legacy "<resourceID>-<metric>" ID.