From ad1e34277d247c8a8907057b221d9ffb9c9e7f51 Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Wed, 7 Oct 2026 16:54:48 +0100 Subject: [PATCH 1/3] Stop alert config saves writing into the live config SaveAlertConfig normalized the config it was handed in place, and its callers hand it alerts.Manager.GetConfig(), a shallow copy whose maps and threshold pointers were the running config's own. Alert activation and the alert identity migration therefore wrote the live delay map and threshold pointers outside the manager lock while evaluation read them. The shallow copy also let UpdateConfig, which normalizes the override map it is handed in place, rewrite the live overrides while GET /api/alerts/config encoded another snapshot. A concurrent map read or iteration plus a write is a fatal Go runtime error, so these paths could kill Pulse. AlertConfig.Clone is a typed deep copy. GetConfig returns one, so callers own what they encode, persist and edit, and SaveAlertConfig clones before normalizing, because a config just applied with UpdateConfig is the live config. GetConfig has no per-resource caller (once per poll cycle, backup check, request or Patrol run); a clone with 1,000 overrides takes about 0.3 ms. A test fills every field and fails when a new map, slice or pointer is not copied. Persistence also stored and loaded an explicit 0 type-level alert delay ("no delay") as the 5-second factory delay, which the noisy-gauge rule stretches to 300 seconds for memory and temperature, while the manager keeps 0. The shared map write was what made the running manager agree with the file; without it, memory would say 0 and disk 5. Persistence now keeps 0 on save and load like NormalizeTimeThresholds, and only a missing or negative delay takes the default. Pulse saves never wrote a 0, and the first v6 start re-saves alerts.json, so only a newly sent or hand-edited 0 changes meaning. On main, a reproducer taking the activation, migration and GET steps against live evaluation died with "fatal error: concurrent map iteration and map write" in 5 of 5 plain runs and reported 16 races under -race. It passes with this change. --- .../v6/internal/subsystems/alerts.md | 22 +++ internal/alerts/config/clone.go | 164 +++++++++++++++++ internal/alerts/config/clone_test.go | 133 ++++++++++++++ internal/alerts/config_runtime.go | 8 +- .../alerts/resolved_lock_discipline_test.go | 79 ++++++++ internal/config/persistence.go | 12 +- .../persistence_alert_ownership_test.go | 171 ++++++++++++++++++ .../persistence_alerts_normalization_test.go | 11 +- internal/config/persistence_test.go | 8 +- 9 files changed, 596 insertions(+), 12 deletions(-) create mode 100644 internal/alerts/config/clone.go create mode 100644 internal/alerts/config/clone_test.go create mode 100644 internal/config/persistence_alert_ownership_test.go diff --git a/docs/release-control/v6/internal/subsystems/alerts.md b/docs/release-control/v6/internal/subsystems/alerts.md index a7cb638119..bc0caa053f 100644 --- a/docs/release-control/v6/internal/subsystems/alerts.md +++ b/docs/release-control/v6/internal/subsystems/alerts.md @@ -3121,6 +3121,28 @@ ownership is `internal/alerts/alert_identity_migration_test.go`, `internal/monitoring/monitor_alert_override_migration_test.go`, and `frontend-modern/src/features/alerts/__tests__/alertsConfigurationModel.snapshot.test.ts`. +### Alert config snapshots are owned copies + +`Manager.GetConfig()` returns `AlertConfig.Clone()`, a typed deep copy that +shares no map, slice or pointer with the live config (custom rule filter +values are copied as decoded JSON values). Callers encode, persist +and edit that snapshot outside `m.mu`: the GET config handler, alert +activation, the identity migration, backup evaluation and diagnostics. It used +to be a shallow copy, so `SaveAlertConfig` normalizing it in place, or +`UpdateConfig` re-normalizing the overrides of a re-applied snapshot, wrote the +live maps while evaluation or another encoder read them, and Go aborted Pulse +with a concurrent map write. `SaveAlertConfig` also clones before normalizing, +because a config just handed to `UpdateConfig` is the live config. Persistence +keeps an explicit `0` type-level delay as no delay on save and load, matching +`NormalizeTimeThresholds`. It used to store and load it as the 5-second factory +delay, which the noisy-gauge rule stretches to 300 seconds for memory and +temperature, and the shared map write was what made the running manager agree. +Pulse saves never wrote a `0`, so only a newly sent or hand-edited `0` changes +meaning. `internal/alerts/config/clone_test.go` +fills every field and fails when a new reference field is not copied. +Regression ownership is `internal/alerts/resolved_lock_discipline_test.go` and +`internal/config/persistence_alert_ownership_test.go`. + ### Versioned alert-intent policy The alerts runtime owns one versioned alert-intent document and its durable diff --git a/internal/alerts/config/clone.go b/internal/alerts/config/clone.go new file mode 100644 index 0000000000..388decc3fc --- /dev/null +++ b/internal/alerts/config/clone.go @@ -0,0 +1,164 @@ +package config + +import ( + "maps" + "slices" +) + +// Clone returns a deep copy of the configuration that shares no maps, slices +// or pointers with c, so the copy can be changed, normalized or encoded +// without touching c. Nil and empty maps and slices keep their distinction. +// Custom rule filter values are copied as the JSON values the API and +// alerts.json decode them to; other Go values stored there stay shared. +// +// The alert manager hands out its live configuration only through Clone; +// callers persist, encode and edit what it returns outside the manager lock. +func (c AlertConfig) Clone() AlertConfig { + clone := c + clone.ActivationTime = clonePointer(c.ActivationTime) + clone.GuestDefaults = c.GuestDefaults.Clone() + clone.NodeDefaults = c.NodeDefaults.Clone() + clone.AgentDefaults = c.AgentDefaults.Clone() + clone.DiskFillByType = maps.Clone(c.DiskFillByType) + clone.DiskTempByType = maps.Clone(c.DiskTempByType) + clone.DockerIgnoredContainerPrefixes = slices.Clone(c.DockerIgnoredContainerPrefixes) + clone.IgnoredGuestPrefixes = slices.Clone(c.IgnoredGuestPrefixes) + clone.GuestTagWhitelist = slices.Clone(c.GuestTagWhitelist) + clone.GuestTagBlacklist = slices.Clone(c.GuestTagBlacklist) + clone.PBSDefaults = c.PBSDefaults.Clone() + clone.KubernetesDefaults = c.KubernetesDefaults.Clone() + clone.TrueNASDefaults = c.TrueNASDefaults.Clone() + clone.TrueNASDiskDefaults = c.TrueNASDiskDefaults.Clone() + clone.VMwareDefaults = c.VMwareDefaults.Clone() + clone.BackupDefaults = c.BackupDefaults.clone() + if c.Overrides != nil { + clone.Overrides = make(map[string]ThresholdConfig, len(c.Overrides)) + for id, override := range c.Overrides { + clone.Overrides[id] = override.Clone() + } + } + if c.CustomRules != nil { + clone.CustomRules = make([]CustomAlertRule, len(c.CustomRules)) + for i, rule := range c.CustomRules { + clone.CustomRules[i] = rule.clone() + } + } + clone.Schedule.QuietHours.Days = maps.Clone(c.Schedule.QuietHours.Days) + if c.Schedule.Escalation.Levels != nil { + clone.Schedule.Escalation.Levels = make([]EscalationLevel, len(c.Schedule.Escalation.Levels)) + for i, level := range c.Schedule.Escalation.Levels { + level.DestinationIDs = slices.Clone(level.DestinationIDs) + clone.Schedule.Escalation.Levels[i] = level + } + } + clone.TimeThresholds = maps.Clone(c.TimeThresholds) + clone.MetricTimeThresholds = cloneNestedIntMap(c.MetricTimeThresholds) + clone.MetricEvaluationWindows = cloneNestedIntMap(c.MetricEvaluationWindows) + return clone +} + +// Clone returns a deep copy of the thresholds that shares no pointers with t. +func (t ThresholdConfig) Clone() ThresholdConfig { + clone := t + clone.CPU = clonePointer(t.CPU) + clone.Memory = clonePointer(t.Memory) + clone.Disk = clonePointer(t.Disk) + clone.DiskRead = clonePointer(t.DiskRead) + clone.DiskWrite = clonePointer(t.DiskWrite) + clone.NetworkIn = clonePointer(t.NetworkIn) + clone.NetworkOut = clonePointer(t.NetworkOut) + clone.Usage = clonePointer(t.Usage) + clone.Temperature = clonePointer(t.Temperature) + clone.DiskTemperature = clonePointer(t.DiskTemperature) + clone.SMARTHealthFailure = clonePointer(t.SMARTHealthFailure) + clone.SMARTReallocated = clonePointer(t.SMARTReallocated) + clone.SMARTPending = clonePointer(t.SMARTPending) + clone.SMARTUncorrectable = clonePointer(t.SMARTUncorrectable) + clone.SMARTMediaErrors = clonePointer(t.SMARTMediaErrors) + clone.SMARTCRCErrorDelta = clonePointer(t.SMARTCRCErrorDelta) + clone.SMARTLifeWarning = clonePointer(t.SMARTLifeWarning) + clone.SMARTLifeCritical = clonePointer(t.SMARTLifeCritical) + clone.SMARTSpareWarning = clonePointer(t.SMARTSpareWarning) + clone.SMARTSpareCritical = clonePointer(t.SMARTSpareCritical) + if t.Backup != nil { + backup := t.Backup.clone() + clone.Backup = &backup + } + clone.Snapshot = clonePointer(t.Snapshot) + clone.Note = clonePointer(t.Note) + return clone +} + +func (b BackupAlertConfig) clone() BackupAlertConfig { + clone := b + clone.AlertOrphaned = clonePointer(b.AlertOrphaned) + clone.IgnoreVMIDs = slices.Clone(b.IgnoreVMIDs) + return clone +} + +func (r CustomAlertRule) clone() CustomAlertRule { + clone := r + if r.FilterConditions.Filters != nil { + clone.FilterConditions.Filters = make([]FilterCondition, len(r.FilterConditions.Filters)) + for i, filter := range r.FilterConditions.Filters { + filter.Value = cloneJSONValue(filter.Value) + clone.FilterConditions.Filters[i] = filter + } + } + clone.Thresholds = r.Thresholds.Clone() + if r.Notifications.Email != nil { + email := *r.Notifications.Email + email.Recipients = slices.Clone(email.Recipients) + clone.Notifications.Email = &email + } + clone.Notifications.Webhook = clonePointer(r.Notifications.Webhook) + return clone +} + +// clonePointer copies the value p points at. Only use it for types that hold +// no maps, slices or pointers of their own. +func clonePointer[T any](p *T) *T { + if p == nil { + return nil + } + clone := *p + return &clone +} + +func cloneNestedIntMap(input map[string]map[string]int) map[string]map[string]int { + if input == nil { + return nil + } + clone := make(map[string]map[string]int, len(input)) + for key, inner := range input { + clone[key] = maps.Clone(inner) + } + return clone +} + +// cloneJSONValue copies a filter value as encoding/json decodes one: arrays +// and objects are copied, scalars are values already. +func cloneJSONValue(value interface{}) interface{} { + switch typed := value.(type) { + case []interface{}: + if typed == nil { + return typed + } + clone := make([]interface{}, len(typed)) + for i, item := range typed { + clone[i] = cloneJSONValue(item) + } + return clone + case map[string]interface{}: + if typed == nil { + return typed + } + clone := make(map[string]interface{}, len(typed)) + for key, item := range typed { + clone[key] = cloneJSONValue(item) + } + return clone + default: + return value + } +} diff --git a/internal/alerts/config/clone_test.go b/internal/alerts/config/clone_test.go new file mode 100644 index 0000000000..4afc629bf7 --- /dev/null +++ b/internal/alerts/config/clone_test.go @@ -0,0 +1,133 @@ +package config + +import ( + "reflect" + "testing" + "time" +) + +var timeType = reflect.TypeOf(time.Time{}) + +// Every field is filled, so a map, slice or pointer added to the config later +// fails here until Clone copies it. +func TestAlertConfigCloneSharesNothingWithTheOriginal(t *testing.T) { + var original AlertConfig + fillForClone(reflect.ValueOf(&original).Elem()) + + clone := original.Clone() + + if !reflect.DeepEqual(clone, original) { + t.Fatalf("clone differs from the original:\n got %+v\nwant %+v", clone, original) + } + assertNothingShared(t, "AlertConfig", reflect.ValueOf(original), reflect.ValueOf(clone)) +} + +func TestAlertConfigCloneKeepsNilAndEmptyApart(t *testing.T) { + original := AlertConfig{ + Overrides: map[string]ThresholdConfig{}, + CustomRules: []CustomAlertRule{{FilterConditions: FilterStack{Filters: []FilterCondition{{Value: []interface{}(nil)}}}}}, + } + + clone := original.Clone() + + if !reflect.DeepEqual(clone, original) { + t.Fatalf("clone differs from the original:\n got %#v\nwant %#v", clone, original) + } + if clone.Overrides == nil { + t.Fatal("clone turned an empty overrides map into nil") + } + if clone.TimeThresholds != nil { + t.Fatal("clone turned a nil delay map into an empty one") + } +} + +func fillForClone(value reflect.Value) { + switch value.Kind() { + case reflect.Bool: + value.SetBool(true) + case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: + value.SetInt(7) + case reflect.Float32, reflect.Float64: + value.SetFloat(7.5) + case reflect.String: + value.SetString("filled") + case reflect.Pointer: + value.Set(reflect.New(value.Type().Elem())) + fillForClone(value.Elem()) + case reflect.Map: + value.Set(reflect.MakeMap(value.Type())) + key := reflect.New(value.Type().Key()).Elem() + fillForClone(key) + item := reflect.New(value.Type().Elem()).Elem() + fillForClone(item) + value.SetMapIndex(key, item) + case reflect.Slice: + value.Set(reflect.MakeSlice(value.Type(), 1, 1)) + fillForClone(value.Index(0)) + case reflect.Interface: + // Filter values arrive from encoding/json. + value.Set(reflect.ValueOf(map[string]interface{}{"filled": []interface{}{"filled", 7.5}})) + case reflect.Struct: + if value.Type() == timeType { + value.Set(reflect.ValueOf(time.Date(2026, 10, 7, 12, 0, 0, 0, time.UTC))) + return + } + for i := 0; i < value.NumField(); i++ { + if value.Field(i).CanSet() { + fillForClone(value.Field(i)) + } + } + } +} + +func assertNothingShared(t *testing.T, path string, original, clone reflect.Value) { + t.Helper() + switch original.Kind() { + case reflect.Pointer: + if original.IsNil() { + return + } + if original.Pointer() == clone.Pointer() { + t.Errorf("%s: clone shares the pointer", path) + return + } + assertNothingShared(t, path, original.Elem(), clone.Elem()) + case reflect.Map: + if original.IsNil() { + return + } + if original.Pointer() == clone.Pointer() { + t.Errorf("%s: clone shares the map", path) + return + } + iter := original.MapRange() + for iter.Next() { + assertNothingShared(t, path+"[key]", iter.Value(), clone.MapIndex(iter.Key())) + } + case reflect.Slice: + if original.Len() == 0 { + return + } + if original.Pointer() == clone.Pointer() { + t.Errorf("%s: clone shares the slice", path) + return + } + for i := 0; i < original.Len(); i++ { + assertNothingShared(t, path+"[i]", original.Index(i), clone.Index(i)) + } + case reflect.Interface: + if original.IsNil() { + return + } + assertNothingShared(t, path, original.Elem(), clone.Elem()) + case reflect.Struct: + if original.Type() == timeType { + return + } + for i := 0; i < original.NumField(); i++ { + if original.Type().Field(i).IsExported() { + assertNothingShared(t, path+"."+original.Type().Field(i).Name, original.Field(i), clone.Field(i)) + } + } + } +} diff --git a/internal/alerts/config_runtime.go b/internal/alerts/config_runtime.go index 3b6259e71e..6b12269ca4 100644 --- a/internal/alerts/config_runtime.go +++ b/internal/alerts/config_runtime.go @@ -634,11 +634,15 @@ func (m *Manager) reevaluateActiveAlertsLocked() { } } -// GetConfig returns the current alert configuration. +// GetConfig returns a copy of the alert configuration that the caller owns. +// Callers encode, persist and edit it outside m.mu, and UpdateConfig +// normalizes the maps it is handed in place, so a copy that shared the live +// config's maps let a save or a re-apply write them while evaluation read +// them, which Go aborts as a concurrent map write. func (m *Manager) GetConfig() AlertConfig { m.mu.RLock() defer m.mu.RUnlock() - return m.config + return m.config.Clone() } func cloneThreshold(threshold *HysteresisThreshold) *HysteresisThreshold { diff --git a/internal/alerts/resolved_lock_discipline_test.go b/internal/alerts/resolved_lock_discipline_test.go index 311338a8e0..b746dafdee 100644 --- a/internal/alerts/resolved_lock_discipline_test.go +++ b/internal/alerts/resolved_lock_discipline_test.go @@ -1,6 +1,7 @@ package alerts import ( + "encoding/json" "fmt" "runtime" "sync" @@ -8,6 +9,7 @@ import ( "time" alertspecs "github.com/rcourtman/pulse-go-rewrite/internal/alerts/specs" + "github.com/rcourtman/pulse-go-rewrite/internal/models" "github.com/rcourtman/pulse-go-rewrite/internal/unifiedresources" "github.com/rs/zerolog" ) @@ -304,3 +306,80 @@ func waitForResolvedConcurrencyGroup(t *testing.T, wg *sync.WaitGroup) { t.Fatal("resolved alert concurrency paths deadlocked") } } + +// GetConfig snapshots are encoded, persisted and edited outside m.mu, and +// UpdateConfig normalizes the maps it is handed in place. While a snapshot +// shared the live config's maps, re-applying one rewrote the override map +// that the GET /api/alerts/config handler was encoding from another snapshot, +// and Go aborted the process with a concurrent map write. Run with -race. +func TestConfigSnapshotsShareNothingWithTheLiveConfig(t *testing.T) { + originalLogLevel := zerolog.GlobalLevel() + zerolog.SetGlobalLevel(zerolog.Disabled) + t.Cleanup(func() { + zerolog.SetGlobalLevel(originalLogLevel) + }) + + m := NewManagerWithDataDir(t.TempDir()) + t.Cleanup(m.Stop) + + seed := m.GetConfig() + seed.Enabled = true + seed.ActivationState = ActivationActive + seed.Overrides = make(map[string]ThresholdConfig) + for i := 0; i < 64; i++ { + seed.Overrides[fmt.Sprintf("agent:snapshot-%d", i)] = ThresholdConfig{ + CPU: &HysteresisThreshold{Trigger: 95, Clear: 90}, + } + } + m.UpdateConfig(seed) + + snapshot := m.GetConfig() + snapshot.Overrides["agent:snapshot-0"].CPU.Trigger = 1 + delete(snapshot.Overrides, "agent:snapshot-1") + snapshot.TimeThresholds["guest"] = 99 + snapshot.AgentDefaults.CPU.Trigger = 1 + + live := m.GetConfig() + if got := live.Overrides["agent:snapshot-0"].CPU.Trigger; got != 95 { + t.Fatalf("editing a snapshot changed the live override trigger to %v", got) + } + if _, ok := live.Overrides["agent:snapshot-1"]; !ok { + t.Fatal("deleting from a snapshot removed the live override") + } + if live.TimeThresholds["guest"] == 99 { + t.Fatal("editing a snapshot changed the live guest delay") + } + if live.AgentDefaults.CPU.Trigger == 1 { + t.Fatal("editing a snapshot changed the live agent CPU trigger") + } + + var wg sync.WaitGroup + wg.Add(3) + go func() { + defer wg.Done() + for i := 0; i < 200; i++ { + m.UpdateConfig(m.GetConfig()) + } + }() + go func() { + defer wg.Done() + for i := 0; i < 200; i++ { + if _, err := json.Marshal(m.GetConfig()); err != nil { + t.Errorf("encode snapshot: %v", err) + return + } + } + }() + go func() { + defer wg.Done() + for i := 0; i < 200; i++ { + id := fmt.Sprintf("snapshot-%d", i%64) + m.CheckHost(models.Host{ID: id, Hostname: id, Status: "online", CPUUsage: 50}) + } + }() + wg.Wait() + + if got := len(m.GetConfig().Overrides); got != 64 { + t.Fatalf("live overrides = %d after re-applying snapshots, want 64", got) + } +} diff --git a/internal/config/persistence.go b/internal/config/persistence.go index c1eefbf941..359e2b9af7 100644 --- a/internal/config/persistence.go +++ b/internal/config/persistence.go @@ -882,8 +882,11 @@ func normalizeAlertDefaults(config *alerts.AlertConfig) { config.TimeThresholds = make(map[string]int) } defaultDelay := 5 + // Zero means no delay, as the alert manager reads it + // (alerts.Manager.UpdateConfig keeps it); only a missing or negative + // delay takes the default. ensureDelay := func(key string) { - if delay, ok := config.TimeThresholds[key]; !ok || delay <= 0 { + if delay, ok := config.TimeThresholds[key]; !ok || delay < 0 { config.TimeThresholds[key] = defaultDelay } } @@ -892,7 +895,7 @@ func normalizeAlertDefaults(config *alerts.AlertConfig) { ensureDelay("agent") ensureDelay("storage") ensureDelay("pbs") - if delay, ok := config.TimeThresholds["all"]; ok && delay <= 0 { + if delay, ok := config.TimeThresholds["all"]; ok && delay < 0 { config.TimeThresholds["all"] = defaultDelay } @@ -962,6 +965,11 @@ func (c *ConfigPersistence) SaveAlertConfig(config alerts.AlertConfig) error { c.mu.Lock() defer c.mu.Unlock() + // Normalize a copy. Callers hand in alerts.Manager.GetConfig() snapshots + // and configs they have just applied to the manager, so normalizing their + // maps and threshold pointers in place wrote into the running config. + config = config.Clone() + // Save-specific: normalize override clear values before shared defaults run if config.MinimumDelta <= 0 { margin := config.HysteresisMargin diff --git a/internal/config/persistence_alert_ownership_test.go b/internal/config/persistence_alert_ownership_test.go new file mode 100644 index 0000000000..1b83aea3c0 --- /dev/null +++ b/internal/config/persistence_alert_ownership_test.go @@ -0,0 +1,171 @@ +package config_test + +import ( + "encoding/json" + "fmt" + "reflect" + "sync" + "testing" + + "github.com/rcourtman/pulse-go-rewrite/internal/alerts" + "github.com/rcourtman/pulse-go-rewrite/internal/config" + "github.com/rcourtman/pulse-go-rewrite/internal/models" + "github.com/rs/zerolog" +) + +// SaveAlertConfig used to normalize the config it was handed in place. +// Callers hand it alerts.Manager.GetConfig() snapshots, whose maps and +// threshold pointers were the live manager config's own, so every save +// wrote into the running config outside the manager lock. Evaluation reads +// those maps under the lock and the GET handler encodes a snapshot outside +// it, so a save racing either one could abort Pulse with a concurrent map +// write. Run with -race: the rounds below take the same steps as alert +// activation, the identity migration and GET /api/alerts/config, while +// evaluation runs. +func TestSaveAlertConfigLeavesTheLiveManagerConfigAlone(t *testing.T) { + originalLevel := zerolog.GlobalLevel() + zerolog.SetGlobalLevel(zerolog.Disabled) + t.Cleanup(func() { zerolog.SetGlobalLevel(originalLevel) }) + + manager := alerts.NewManagerWithDataDir(t.TempDir()) + t.Cleanup(manager.Stop) + + seed := manager.GetConfig() + seed.Enabled = true + seed.ActivationState = alerts.ActivationActive + // Agent CPU alerts off: persistence rewrites the clear value of a zero + // trigger, so a shared threshold pointer is written on every save. + seed.AgentDefaults.CPU = &alerts.HysteresisThreshold{Trigger: 0, Clear: 0} + // No delay for guests: persistence used to replace an explicit zero with + // five seconds, writing the live delay map. + seed.TimeThresholds = map[string]int{"guest": 0} + seed.Overrides = make(map[string]alerts.ThresholdConfig) + for i := 0; i < 64; i++ { + seed.Overrides[fmt.Sprintf("agent:host-%d", i)] = alerts.ThresholdConfig{ + CPU: &alerts.HysteresisThreshold{Trigger: 95, Clear: 90}, + } + } + manager.UpdateConfig(seed) + want := manager.GetConfig() + wantJSON, err := json.Marshal(want) + if err != nil { + t.Fatalf("marshal seeded config: %v", err) + } + + persistence := config.NewConfigPersistence(t.TempDir()) + if err := persistence.EnsureConfigDir(); err != nil { + t.Fatalf("EnsureConfigDir: %v", err) + } + + const rounds = 200 + var wg sync.WaitGroup + run := func(step func(round int)) { + wg.Add(1) + go func() { + defer wg.Done() + for round := 0; round < rounds; round++ { + step(round) + } + }() + } + // POST /api/alerts/activate: apply a snapshot, then persist it. + run(func(int) { + cfg := manager.GetConfig() + manager.UpdateConfig(cfg) + if err := persistence.SaveAlertConfig(cfg); err != nil { + t.Errorf("save after update: %v", err) + } + }) + // The alert identity migration persists a snapshot. + run(func(int) { + if err := persistence.SaveAlertConfig(manager.GetConfig()); err != nil { + t.Errorf("save snapshot: %v", err) + } + }) + // GET /api/alerts/config encodes a snapshot outside the manager lock. + run(func(int) { + if _, err := json.Marshal(manager.GetConfig()); err != nil { + t.Errorf("encode snapshot: %v", err) + } + }) + run(func(round int) { + id := fmt.Sprintf("host-%d", round%64) + manager.CheckHost(models.Host{ID: id, Hostname: id, Status: "online", CPUUsage: 50}) + }) + wg.Wait() + + got := manager.GetConfig() + if got.TimeThresholds["guest"] != 0 { + t.Fatalf("live guest delay = %d after saves, want the configured 0", got.TimeThresholds["guest"]) + } + gotJSON, err := json.Marshal(got) + if err != nil { + t.Fatalf("marshal final config: %v", err) + } + if string(gotJSON) != string(wantJSON) { + t.Fatalf("saving changed the live config:\n got %s\nwant %s", gotJSON, wantJSON) + } + + loaded, err := persistence.LoadAlertConfig() + if err != nil { + t.Fatalf("LoadAlertConfig: %v", err) + } + if delay, ok := loaded.TimeThresholds["guest"]; !ok || delay != 0 { + t.Fatalf("persisted guest delay = %d (present %v), want the configured 0", delay, ok) + } +} + +func TestSaveAlertConfigDoesNotNormalizeTheCallersConfig(t *testing.T) { + persistence := config.NewConfigPersistence(t.TempDir()) + if err := persistence.EnsureConfigDir(); err != nil { + t.Fatalf("EnsureConfigDir: %v", err) + } + + cfg := alerts.AlertConfig{ + AgentDefaults: alerts.ThresholdConfig{ + CPU: &alerts.HysteresisThreshold{Trigger: 80, Clear: 0}, + }, + TimeThresholds: map[string]int{"guest": -1, "node": 0}, + Overrides: map[string]alerts.ThresholdConfig{ + "storage:local": {Usage: &alerts.HysteresisThreshold{Trigger: 90, Clear: 0}}, + }, + BackupDefaults: alerts.BackupAlertConfig{IgnoreVMIDs: []string{" 100 ", "100"}}, + } + before, err := json.Marshal(cfg) + if err != nil { + t.Fatalf("marshal: %v", err) + } + cpu, override := cfg.AgentDefaults.CPU, cfg.Overrides["storage:local"].Usage + + if err := persistence.SaveAlertConfig(cfg); err != nil { + t.Fatalf("SaveAlertConfig: %v", err) + } + + after, err := json.Marshal(cfg) + if err != nil { + t.Fatalf("marshal: %v", err) + } + if string(after) != string(before) { + t.Fatalf("SaveAlertConfig changed its argument:\n got %s\nwant %s", after, before) + } + if cfg.AgentDefaults.CPU != cpu || cfg.Overrides["storage:local"].Usage != override { + t.Fatal("SaveAlertConfig replaced the caller's threshold pointers") + } + + loaded, err := persistence.LoadAlertConfig() + if err != nil { + t.Fatalf("LoadAlertConfig: %v", err) + } + if got := loaded.TimeThresholds; got["guest"] != 5 || got["node"] != 0 { + t.Fatalf("persisted delays = %v, want guest 5 (unset) and node 0 (no delay)", got) + } + if got := *loaded.AgentDefaults.CPU; got != (alerts.HysteresisThreshold{Trigger: 80, Clear: 75}) { + t.Fatalf("persisted agent CPU = %+v, want clear derived below the trigger", got) + } + if got := *loaded.Overrides["storage:local"].Usage; got != (alerts.HysteresisThreshold{Trigger: 90, Clear: 85}) { + t.Fatalf("persisted override usage = %+v, want clear derived below the trigger", got) + } + if want := []string{"100"}; !reflect.DeepEqual(loaded.BackupDefaults.IgnoreVMIDs, want) { + t.Fatalf("persisted ignore list = %v, want %v", loaded.BackupDefaults.IgnoreVMIDs, want) + } +} diff --git a/internal/config/persistence_alerts_normalization_test.go b/internal/config/persistence_alerts_normalization_test.go index 33f768ea66..84eee6dc1f 100644 --- a/internal/config/persistence_alerts_normalization_test.go +++ b/internal/config/persistence_alerts_normalization_test.go @@ -213,7 +213,7 @@ func TestLoadAlertConfig_Normalization(t *testing.T) { }, }, { - name: "TimeThresholds defaults", + name: "TimeThresholds zero means no delay", input: map[string]interface{}{ "timeThreshold": 0, "timeThresholds": map[string]interface{}{ @@ -222,8 +222,9 @@ func TestLoadAlertConfig_Normalization(t *testing.T) { }, }, verify: func(t *testing.T, cfg *alerts.AlertConfig) { - assert.Equal(t, 5, cfg.TimeThresholds["guest"]) - assert.Equal(t, 5, cfg.TimeThresholds["all"]) + assert.Equal(t, 0, cfg.TimeThresholds["guest"]) + assert.Equal(t, 0, cfg.TimeThresholds["all"]) + assert.Equal(t, 5, cfg.TimeThresholds["node"]) }, }, { @@ -290,11 +291,13 @@ func TestLoadAlertConfig_Normalization(t *testing.T) { "timeThresholds": map[string]interface{}{ "guest": -1, "pbs": 0, + "all": -1, }, }, verify: func(t *testing.T, cfg *alerts.AlertConfig) { assert.Equal(t, 5, cfg.TimeThresholds["guest"]) - assert.Equal(t, 5, cfg.TimeThresholds["pbs"]) + assert.Equal(t, 0, cfg.TimeThresholds["pbs"]) + assert.Equal(t, 5, cfg.TimeThresholds["all"]) }, }, { diff --git a/internal/config/persistence_test.go b/internal/config/persistence_test.go index 21ef33a6b2..4da499c49e 100644 --- a/internal/config/persistence_test.go +++ b/internal/config/persistence_test.go @@ -397,7 +397,7 @@ func TestLoadAlertConfigAppliesDefaults(t *testing.T) { raw := alerts.AlertConfig{ Enabled: false, - TimeThresholds: map[string]int{"guest": 0, "node": 0}, + TimeThresholds: map[string]int{"guest": -1, "node": 0}, DockerIgnoredContainerPrefixes: []string{" Runner "}, SnapshotDefaults: alerts.SnapshotAlertConfig{ Enabled: true, @@ -432,10 +432,10 @@ func TestLoadAlertConfigAppliesDefaults(t *testing.T) { } if got := loaded.TimeThresholds["guest"]; got != 5 { - t.Fatalf("expected guest threshold default 5, got %d", got) + t.Fatalf("expected unset guest threshold default 5, got %d", got) } - if got := loaded.TimeThresholds["node"]; got != 5 { - t.Fatalf("expected node threshold default 5, got %d", got) + if got := loaded.TimeThresholds["node"]; got != 0 { + t.Fatalf("expected node threshold 0 (no delay) to be kept, got %d", got) } if loaded.NodeDefaults.Temperature == nil { t.Fatalf("expected node temperature defaults to be set") From 71a470129f27408288cff953b75f84bcfee75265 Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Wed, 7 Oct 2026 17:10:13 +0100 Subject: [PATCH 2/3] Stop alert evaluation writing live thresholds outside the lock Host and guest evaluation filled in the Clear of a per-disk override's threshold after dropping m.mu, writing through the live config's pointer. GetConfig clones that pointer under the read lock, which does not exclude the writer, and the saved override silently took whatever the last poll filled in. Docker evaluation pointed into m.config.DockerDefaults, and read the restart-loop and memory-limit settings, with no lock held while UpdateConfig replaced m.config. Clone the override's Disk threshold under m.mu before normalizing it, resolve Docker container thresholds from cloned defaults plus the override under the lock, and read the restart-loop and memory-limit settings under it. Drop getThresholdForMetricFromConfig: nothing has called it since the v6 split, and it normalized in place. TestAlertEvaluationLeavesTheLiveConfigToTheLock runs host, guest and Docker evaluation against GetConfig() and UpdateConfig(GetConfig()) under -race. With host.go, guest.go, docker.go and metric_runtime.go overlaid by their origin/main copies it fails: the live override's clear reads 85, and -race reports every site above. Stacked on #2692, which made GetConfig return an owned copy. --- .../v6/internal/subsystems/alerts.md | 20 +++ internal/alerts/alerts_test.go | 95 ++++++++++++ internal/alerts/docker.go | 29 ++-- internal/alerts/guest.go | 3 + internal/alerts/host.go | 4 +- internal/alerts/metric_runtime.go | 10 -- internal/alerts/utility_test.go | 145 ------------------ 7 files changed, 137 insertions(+), 169 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/alerts.md b/docs/release-control/v6/internal/subsystems/alerts.md index bc0caa053f..6e4f87d46f 100644 --- a/docs/release-control/v6/internal/subsystems/alerts.md +++ b/docs/release-control/v6/internal/subsystems/alerts.md @@ -3143,6 +3143,26 @@ fills every field and fails when a new reference field is not copied. Regression ownership is `internal/alerts/resolved_lock_discipline_test.go` and `internal/config/persistence_alert_ownership_test.go`. +### Alert evaluation owns the thresholds it normalizes + +Host, guest and Docker container evaluation copy what they read from +`m.config` while holding `m.mu`, and only then fill in defaults. +`CheckHost` and `CheckGuest` clone a per-disk +override's `Disk` threshold before `ensureHysteresisThreshold` sets a missing +`Clear` to the trigger minus 5. They used to normalize the live override's +pointer after unlocking, so a `GetConfig()` clone under the read lock raced +the write, and the saved override changed to whatever the last poll filled in. +The live override now keeps the clear it was saved with, and the config API +returns that value. Docker container evaluation resolves its thresholds from +cloned `DockerDefaults` plus the override under the same lock, and reads the +restart-loop and memory-limit settings under the lock. It used to point into +`m.config.DockerDefaults` with no lock while `UpdateConfig` replaced +`m.config`. The test-only `getThresholdForMetricFromConfig`, which normalized +in place, is gone. `internal/alerts/alerts_test.go` +`TestAlertEvaluationLeavesTheLiveConfigToTheLock` runs host, guest and Docker +evaluation against `GetConfig()` and `UpdateConfig(GetConfig())` under +`-race`. + ### Versioned alert-intent policy The alerts runtime owns one versioned alert-intent document and its durable diff --git a/internal/alerts/alerts_test.go b/internal/alerts/alerts_test.go index f0ad060e0f..dea4577c90 100644 --- a/internal/alerts/alerts_test.go +++ b/internal/alerts/alerts_test.go @@ -2756,6 +2756,101 @@ func TestCheckBackupsIgnoresVMIDs(t *testing.T) { } } +// Host and guest evaluation used to fill in the Clear of a disk override's +// threshold after dropping m.mu, writing through the live config's pointer +// while GetConfig cloned it under the read lock, and Docker evaluation read +// m.config.DockerDefaults with no lock while UpdateConfig replaced m.config. +// Evaluation now copies what it needs under m.mu. Run with -race. +func TestAlertEvaluationLeavesTheLiveConfigToTheLock(t *testing.T) { + originalLogLevel := zerolog.GlobalLevel() + zerolog.SetGlobalLevel(zerolog.Disabled) + t.Cleanup(func() { + zerolog.SetGlobalLevel(originalLogLevel) + }) + + m := newTestManager(t) + + host := models.Host{ + ID: "eval-lock", + Hostname: "eval-lock", + Status: "online", + Disks: []models.Disk{{Mountpoint: "/", Device: "/dev/sda1", Usage: 50, Total: 100, Used: 50, Free: 50}}, + } + hostDiskOverrideID, _ := hostDiskResourceID(host, host.Disks[0]) + vm := models.VM{ + ID: BuildGuestKey("pve1", "node1", 101), + VMID: 101, + Name: "eval-lock", + Node: "node1", + Instance: "pve1", + Status: "running", + Disks: []models.Disk{{Mountpoint: "/boot", Device: "/dev/vda1", Usage: 50, Total: 100, Used: 50, Free: 50}}, + } + guestDiskOverrideID := "guest-disk:guest:pve1:101/disk:boot-dev-vda1" + dockerHost := models.DockerHost{ + ID: "eval-lock-docker", + Hostname: "eval-lock-docker", + DisplayName: "eval-lock-docker", + Containers: []models.DockerContainer{{ + ID: "abcdef123456", + Name: "web", + State: "running", + CPUPercent: 10, + MemoryPercent: 10, + MemoryUsage: 100 << 20, + MemoryLimit: 1 << 30, + }}, + } + + // A disk override saved with only a trigger leaves Clear for evaluation + // to default. + withTriggerOnlyDiskOverrides := func(cfg AlertConfig) AlertConfig { + cfg.Enabled = true + cfg.ActivationState = ActivationActive + cfg.Overrides = map[string]ThresholdConfig{ + hostDiskOverrideID: {Disk: &HysteresisThreshold{Trigger: 90}}, + guestDiskOverrideID: {Disk: &HysteresisThreshold{Trigger: 90}}, + } + return cfg + } + m.UpdateConfig(withTriggerOnlyDiskOverrides(m.GetConfig())) + + m.CheckHost(host) + m.CheckGuest(vm, "pve1") + live := m.GetConfig() + for _, id := range []string{hostDiskOverrideID, guestDiskOverrideID} { + if got := live.Overrides[id].Disk.Clear; got != 0 { + t.Fatalf("evaluating %s wrote clear %v into the live override", id, got) + } + } + + var wg sync.WaitGroup + wg.Add(3) + go func() { + defer wg.Done() + for i := 0; i < 100; i++ { + // Resetting the overrides gives evaluation a Clear to fill again. + m.UpdateConfig(withTriggerOnlyDiskOverrides(m.GetConfig())) + m.UpdateConfig(m.GetConfig()) + } + }() + go func() { + defer wg.Done() + for i := 0; i < 200; i++ { + _ = m.GetConfig() + } + }() + go func() { + defer wg.Done() + for i := 0; i < 200; i++ { + m.CheckHost(host) + m.CheckGuest(vm, "pve1") + m.CheckDockerHost(dockerHost) + } + }() + wg.Wait() +} + func TestCheckDockerHostIgnoresContainersByPrefix(t *testing.T) { m := newTestManager(t) diff --git a/internal/alerts/docker.go b/internal/alerts/docker.go index 3eaa0d370a..74071b851d 100644 --- a/internal/alerts/docker.go +++ b/internal/alerts/docker.go @@ -352,8 +352,19 @@ func (m *Manager) evaluateDockerContainer(host models.DockerHost, container mode resourceType := "app-container" containerTags := dockerLabelTags(container.Labels) + // Resolve the metric thresholds while the lock is held: the Docker defaults + // are fields of m.config, which UpdateConfig replaces, and the override + // still points into it. applyThresholdOverride clones what it keeps. m.mu.RLock() overrideConfig, hasOverride := m.lookupDockerContainerOverrideNoLock(host.ID, container.Name, resourceID) + thresholds := ThresholdConfig{ + CPU: cloneThreshold(&m.config.DockerDefaults.CPU), + Memory: cloneThreshold(&m.config.DockerDefaults.Memory), + Disk: cloneThreshold(&m.config.DockerDefaults.Disk), + } + if hasOverride { + thresholds = m.applyThresholdOverride(thresholds, overrideConfig) + } m.mu.RUnlock() if hasOverride && overrideConfig.Disabled { // Alerts disabled via override; clear any existing alerts and skip evaluation. @@ -377,16 +388,6 @@ func (m *Manager) evaluateDockerContainer(host models.DockerHost, container mode } else { m.clearDockerContainerStateAlert(resourceID) - // Use Docker-specific defaults for containers - thresholds := ThresholdConfig{ - CPU: &m.config.DockerDefaults.CPU, - Memory: &m.config.DockerDefaults.Memory, - Disk: &m.config.DockerDefaults.Disk, - } - if hasOverride { - thresholds = m.applyThresholdOverride(thresholds, overrideConfig) - } - if thresholds.CPU != nil { cpuCapacityPercent := models.DockerContainerCPUCapacityPercent(container, host.CPUs) cpuMetadata := map[string]interface{}{ @@ -998,6 +999,8 @@ func (m *Manager) checkDockerContainerRestartLoop(host models.DockerHost, contai alertID := fmt.Sprintf("docker-container-restart-loop-%s", resourceID) now := time.Now() + m.mu.Lock() + // Get config values with defaults restartThreshold := m.config.DockerDefaults.RestartCount if restartThreshold == 0 { @@ -1008,8 +1011,6 @@ func (m *Manager) checkDockerContainerRestartLoop(host models.DockerHost, contai timeWindow = 300 // Default: 5 minutes (300 seconds) } - m.mu.Lock() - record, exists := m.dockerRestartTracking[resourceID] if !exists { record = &dockerRestartRecord{ @@ -1164,11 +1165,13 @@ func (m *Manager) checkDockerContainerMemoryLimit(host models.DockerHost, contai alertID := fmt.Sprintf("docker-container-memory-limit-%s", resourceID) // Get config values with defaults + m.mu.RLock() warnThreshold := float64(m.config.DockerDefaults.MemoryWarnPct) + criticalThreshold := float64(m.config.DockerDefaults.MemoryCriticalPct) + m.mu.RUnlock() if warnThreshold == 0 { warnThreshold = 90.0 // Default: 90% } - criticalThreshold := float64(m.config.DockerDefaults.MemoryCriticalPct) if criticalThreshold == 0 { criticalThreshold = 95.0 // Default: 95% } diff --git a/internal/alerts/guest.go b/internal/alerts/guest.go index 25963829ef..edeac41cb3 100644 --- a/internal/alerts/guest.go +++ b/internal/alerts/guest.go @@ -328,8 +328,11 @@ func (m *Manager) CheckGuest(guest any, instanceName string) { 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) m.mu.RUnlock() if hasDiskOverride { if diskOverride.Disabled { diff --git a/internal/alerts/host.go b/internal/alerts/host.go index 65ae1cbd40..8c3edf1d7a 100644 --- a/internal/alerts/host.go +++ b/internal/alerts/host.go @@ -450,9 +450,11 @@ func (m *Manager) CheckHost(host models.Host) { diskResourceID, diskName := hostDiskResourceID(host, disk) seenDisks[diskResourceID] = struct{}{} - // Check for disk-specific override + // 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) m.mu.RUnlock() // Determine the effective disk threshold diff --git a/internal/alerts/metric_runtime.go b/internal/alerts/metric_runtime.go index bfe670920d..112840229b 100644 --- a/internal/alerts/metric_runtime.go +++ b/internal/alerts/metric_runtime.go @@ -86,16 +86,6 @@ func getThresholdForMetric(config ThresholdConfig, metricType string) *Hysteresi } } -// getThresholdForMetricFromConfig returns the threshold for a specific metric type from a ThresholdConfig -// ensuring hysteresis is properly set. -func getThresholdForMetricFromConfig(config ThresholdConfig, metricType string) *HysteresisThreshold { - th := getThresholdForMetric(config, metricType) - if th == nil { - return nil - } - return ensureHysteresisThreshold(th) -} - // getTimeThreshold determines the delay to apply for a metric/resource combination. func (m *Manager) getTimeThreshold(resourceID string, resourceType, metricType string) int { effective := m.resolveEffectiveIntentPolicyNoLock(resourceID, resourceType, MetricAlertIntentSignal(metricType)) diff --git a/internal/alerts/utility_test.go b/internal/alerts/utility_test.go index a4781633cf..f531aab6b5 100644 --- a/internal/alerts/utility_test.go +++ b/internal/alerts/utility_test.go @@ -2469,148 +2469,3 @@ func TestGetThresholdForMetric_EmptyConfig(t *testing.T) { }) } } - -// TestGetThresholdForMetricFromConfig tests the getThresholdForMetricFromConfig function -func TestGetThresholdForMetricFromConfig(t *testing.T) { - // t.Parallel() - - tests := []struct { - name string - config ThresholdConfig - metricType string - wantNil bool - wantTrigger float64 - wantClear float64 - }{ - { - name: "empty config returns nil", - config: ThresholdConfig{}, - metricType: "cpu", - wantNil: true, - }, - { - name: "cpu threshold returned", - config: ThresholdConfig{ - CPU: &HysteresisThreshold{Trigger: 80, Clear: 70}, - }, - metricType: "cpu", - wantTrigger: 80, - wantClear: 70, - }, - { - name: "memory threshold returned", - config: ThresholdConfig{ - Memory: &HysteresisThreshold{Trigger: 85, Clear: 75}, - }, - metricType: "memory", - wantTrigger: 85, - wantClear: 75, - }, - { - name: "disk threshold returned", - config: ThresholdConfig{ - Disk: &HysteresisThreshold{Trigger: 90, Clear: 85}, - }, - metricType: "disk", - wantTrigger: 90, - wantClear: 85, - }, - { - name: "diskRead threshold returned", - config: ThresholdConfig{ - DiskRead: &HysteresisThreshold{Trigger: 50, Clear: 40}, - }, - metricType: "diskRead", - wantTrigger: 50, - wantClear: 40, - }, - { - name: "diskWrite threshold returned", - config: ThresholdConfig{ - DiskWrite: &HysteresisThreshold{Trigger: 55, Clear: 45}, - }, - metricType: "diskWrite", - wantTrigger: 55, - wantClear: 45, - }, - { - name: "networkIn threshold returned", - config: ThresholdConfig{ - NetworkIn: &HysteresisThreshold{Trigger: 70, Clear: 60}, - }, - metricType: "networkIn", - wantTrigger: 70, - wantClear: 60, - }, - { - name: "networkOut threshold returned", - config: ThresholdConfig{ - NetworkOut: &HysteresisThreshold{Trigger: 75, Clear: 65}, - }, - metricType: "networkOut", - wantTrigger: 75, - wantClear: 65, - }, - { - name: "temperature threshold returned", - config: ThresholdConfig{ - Temperature: &HysteresisThreshold{Trigger: 65, Clear: 55}, - }, - metricType: "temperature", - wantTrigger: 65, - wantClear: 55, - }, - { - name: "usage threshold returned", - config: ThresholdConfig{ - Usage: &HysteresisThreshold{Trigger: 88, Clear: 78}, - }, - metricType: "usage", - wantTrigger: 88, - wantClear: 78, - }, - { - name: "unknown metric returns nil", - config: ThresholdConfig{ - CPU: &HysteresisThreshold{Trigger: 80, Clear: 70}, - }, - metricType: "unknown", - wantNil: true, - }, - { - name: "threshold with zero clear gets default hysteresis", - config: ThresholdConfig{ - CPU: &HysteresisThreshold{Trigger: 80, Clear: 0}, - }, - metricType: "cpu", - wantTrigger: 80, - wantClear: 75, // 80 - 5 default margin - }, - } - - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - // t.Parallel() - - result := getThresholdForMetricFromConfig(tc.config, tc.metricType) - - if tc.wantNil { - if result != nil { - t.Errorf("getThresholdForMetricFromConfig() = %v, want nil", result) - } - return - } - - if result == nil { - t.Fatalf("getThresholdForMetricFromConfig() = nil, want non-nil") - } - - if result.Trigger != tc.wantTrigger { - t.Errorf("Trigger = %v, want %v", result.Trigger, tc.wantTrigger) - } - if result.Clear != tc.wantClear { - t.Errorf("Clear = %v, want %v", result.Clear, tc.wantClear) - } - }) - } -} 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 3/3] 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 "-disk-" 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:/disk: