From 4b0f23e0a7a7d03df391f83896cbf4b48657bd24 Mon Sep 17 00:00:00 2001 From: "pulse-triage[bot]" <249995291+pulse-triage[bot]@users.noreply.github.com> Date: Sat, 3 Oct 2026 00:07:05 +0100 Subject: [PATCH 1/2] Distinguish TrueNAS stale polling from failed authentication Preserve existing polling evidence before a connection test updates its ledger. Explain freshness, optional telemetry and session turnover using the read-only projection, correct the 60-second default, and keep diagnostic steps free of credentials and destructive reproductions. Contract-Neutral: Documentation and existing-guide checks only; no runtime, API, transport, identity, access or retention change. Change-source: pulse-maintainer --- docs/TROUBLESHOOTING.md | 6 +- docs/TRUENAS.md | 58 ++++++++++++++++--- .../public/docs/TROUBLESHOOTING.md | 6 +- frontend-modern/public/docs/TRUENAS.md | 58 ++++++++++++++++--- scripts/tests/test_truenas_docs.py | 51 ++++++++++++++++ 5 files changed, 161 insertions(+), 18 deletions(-) diff --git a/docs/TROUBLESHOOTING.md b/docs/TROUBLESHOOTING.md index 55badfaa03..032d808db2 100644 --- a/docs/TROUBLESHOOTING.md +++ b/docs/TROUBLESHOOTING.md @@ -329,8 +329,12 @@ consequential, manually redacted error. transport; TrueNAS 26 removed the former `/api/v2.0` REST endpoints. #### TrueNAS pools/datasets not appearing -- TrueNAS data appears in the unified resource model and may take one polling cycle (30s) to appear. +- TrueNAS data appears in the unified resource model and may take one configured + polling cycle (60 seconds by default) to appear. - Check **Infrastructure** (TrueNAS host), **Storage** (pools/datasets), and **Recovery** (snapshots/replication). +- For data that stops refreshing, use the [TrueNAS polling checks](TRUENAS.md#stale-truenas-data) + before testing or restarting. A stale badge is not proof of an invalid key, + and a successful connection test is not proof that collection has recovered. ### Navigation (v6) diff --git a/docs/TRUENAS.md b/docs/TRUENAS.md index d587106b67..ddd5ea8468 100644 --- a/docs/TRUENAS.md +++ b/docs/TRUENAS.md @@ -9,7 +9,7 @@ Pulse v6 includes first-class monitoring for **TrueNAS SCALE** and **TrueNAS COR 3. Enter the TrueNAS URL (e.g., `https://truenas.local`), the API key, and the username that owns the key. 4. Click **Test Connection** → **Save**. -5. Data appears within one polling cycle (~30 seconds). +5. Data appears within one configured polling cycle (60 seconds by default). ## Creating a TrueNAS API Key @@ -148,7 +148,8 @@ its trusted HTTPS origin without disabling certificate verification. current appliance through the removed `/api/v2.0` REST endpoints. ### No data appearing after adding connection -- Wait at least 30 seconds for the first poll cycle. +- Allow one configured polling cycle (60 seconds by default), not a fixed + 30-second wait. Check that the saved connection is enabled. - **Test Connection** checks authentication and the selected transport, not successful collection of every metric. The legacy REST diagnostic is expected for recognized CORE 13 systems; it is not itself a connection error. @@ -202,12 +203,53 @@ Do not upload a full browser network capture or paste code into the browser console to collect this evidence. ### Stale TrueNAS data -- If TrueNAS data stops updating, the source status transitions to `stale` after ~120 seconds. -- Check TrueNAS connectivity and API key validity. -- In your signed-in Pulse browser session, open `/api/resources` on the same - Pulse origin and inspect the entries with `platformType: "truenas"`. This - uses your existing session without copying a token or cookie. Do not post - the full response; use the sanitized export or a reviewed relevant excerpt. + +A **Pending** or **Stale** badge is not an authentication diagnosis. Keep the +connection and key unchanged while checking whether ordinary polling still +collects data; a working connection can have missing telemetry or a stalled +collection step. + +1. **Record the existing state before testing or restarting.** In your + signed-in Pulse admin browser session, open `/api/truenas/connections` on + the same Pulse origin. Inspect only the affected connection. This is a + read-only request using your existing session; do not copy a token or cookie. +2. Note `enabled`, `poll.intervalSeconds`, `poll.lastAttemptAt`, + `poll.lastSuccessAt`, `poll.consecutiveFailures`, `poll.lastError`, and + `observed.collectedAt`. When present, also note `transport.mode`, + `transport.reconnects` and `transport.lastError`. If your build does not + expose a field, record that rather than interpreting its absence as success. +3. Compare the timestamps after two configured polling cycles, without + pressing **Test Connection**, saving changes or restarting between reads. + Slow requests can take longer than the interval. `lastAttemptAt` records a + completed attempt, not an in-flight request; a frozen value alone cannot + distinguish a blocked request from polling that has not run. + +| Existing evidence | What it distinguishes and what to check next | +|---|---| +| `lastAttemptAt` advances, but `lastSuccessAt` and `observed.collectedAt` do not; failures increase | Polling returns failures. Use the actual error category and method to distinguish TLS, authentication, permissions and collection errors; a stale badge alone does not distinguish them. | +| `observed.collectedAt` advances, but some usage or History panels stay empty | Inventory is refreshing, not necessarily every metric. Follow the [missing-telemetry checks](#inventory-works-but-cpu-memory-or-history-is-missing); do not replace a working key to populate a chart. | +| No timestamps advance, or no completed attempt is recorded | Check that the connection and integration are enabled, then preserve the bounded local error above. This is not proof of an invalid key. | +| Reconnects or TrueNAS sign-ins increase while `observed.collectedAt` advances | Session turnover alone does not establish failed authentication or stopped polling. Record the timing and any existing close/error message separately from the data freshness. | + +**Test Connection** checks connectivity and authentication, not a complete +poll. Testing a saved connection can update `lastAttemptAt` and `lastSuccessAt` +and reset its failure count without refreshing `observed.collectedAt`. Record +any manual test and its time separately; a successful test is not evidence +that missing data or stopped polling has recovered. + +An `app.stats` error such as **Apps are not available** concerns Apps +collection, not necessarily the key. Check the existing Apps status in +TrueNAS, but do not stop Apps or reproduce a hang to gather evidence. Preserve +the existing error before changing anything. Do not repeatedly restart Pulse, +clear History, rotate keys or disable TLS verification as a diagnostic shortcut. + +For a report, include the running Pulse and TrueNAS versions, configured poll +interval, affected panels, before/after timestamps and a reviewed, bounded +error excerpt from the local logs above. Remove private names and addresses +from errors. Do not post the full connection response, credential files, +authentication messages, cookies or a full browser network capture. If safe +inspection is unavailable, describe what you could observe without repeating +the failure or collecting a larger dump. ### Disabling TrueNAS integration Set `PULSE_ENABLE_TRUENAS=false` and restart Pulse. Existing connection data is preserved but polling stops. diff --git a/frontend-modern/public/docs/TROUBLESHOOTING.md b/frontend-modern/public/docs/TROUBLESHOOTING.md index 55badfaa03..032d808db2 100644 --- a/frontend-modern/public/docs/TROUBLESHOOTING.md +++ b/frontend-modern/public/docs/TROUBLESHOOTING.md @@ -329,8 +329,12 @@ consequential, manually redacted error. transport; TrueNAS 26 removed the former `/api/v2.0` REST endpoints. #### TrueNAS pools/datasets not appearing -- TrueNAS data appears in the unified resource model and may take one polling cycle (30s) to appear. +- TrueNAS data appears in the unified resource model and may take one configured + polling cycle (60 seconds by default) to appear. - Check **Infrastructure** (TrueNAS host), **Storage** (pools/datasets), and **Recovery** (snapshots/replication). +- For data that stops refreshing, use the [TrueNAS polling checks](TRUENAS.md#stale-truenas-data) + before testing or restarting. A stale badge is not proof of an invalid key, + and a successful connection test is not proof that collection has recovered. ### Navigation (v6) diff --git a/frontend-modern/public/docs/TRUENAS.md b/frontend-modern/public/docs/TRUENAS.md index d587106b67..ddd5ea8468 100644 --- a/frontend-modern/public/docs/TRUENAS.md +++ b/frontend-modern/public/docs/TRUENAS.md @@ -9,7 +9,7 @@ Pulse v6 includes first-class monitoring for **TrueNAS SCALE** and **TrueNAS COR 3. Enter the TrueNAS URL (e.g., `https://truenas.local`), the API key, and the username that owns the key. 4. Click **Test Connection** → **Save**. -5. Data appears within one polling cycle (~30 seconds). +5. Data appears within one configured polling cycle (60 seconds by default). ## Creating a TrueNAS API Key @@ -148,7 +148,8 @@ its trusted HTTPS origin without disabling certificate verification. current appliance through the removed `/api/v2.0` REST endpoints. ### No data appearing after adding connection -- Wait at least 30 seconds for the first poll cycle. +- Allow one configured polling cycle (60 seconds by default), not a fixed + 30-second wait. Check that the saved connection is enabled. - **Test Connection** checks authentication and the selected transport, not successful collection of every metric. The legacy REST diagnostic is expected for recognized CORE 13 systems; it is not itself a connection error. @@ -202,12 +203,53 @@ Do not upload a full browser network capture or paste code into the browser console to collect this evidence. ### Stale TrueNAS data -- If TrueNAS data stops updating, the source status transitions to `stale` after ~120 seconds. -- Check TrueNAS connectivity and API key validity. -- In your signed-in Pulse browser session, open `/api/resources` on the same - Pulse origin and inspect the entries with `platformType: "truenas"`. This - uses your existing session without copying a token or cookie. Do not post - the full response; use the sanitized export or a reviewed relevant excerpt. + +A **Pending** or **Stale** badge is not an authentication diagnosis. Keep the +connection and key unchanged while checking whether ordinary polling still +collects data; a working connection can have missing telemetry or a stalled +collection step. + +1. **Record the existing state before testing or restarting.** In your + signed-in Pulse admin browser session, open `/api/truenas/connections` on + the same Pulse origin. Inspect only the affected connection. This is a + read-only request using your existing session; do not copy a token or cookie. +2. Note `enabled`, `poll.intervalSeconds`, `poll.lastAttemptAt`, + `poll.lastSuccessAt`, `poll.consecutiveFailures`, `poll.lastError`, and + `observed.collectedAt`. When present, also note `transport.mode`, + `transport.reconnects` and `transport.lastError`. If your build does not + expose a field, record that rather than interpreting its absence as success. +3. Compare the timestamps after two configured polling cycles, without + pressing **Test Connection**, saving changes or restarting between reads. + Slow requests can take longer than the interval. `lastAttemptAt` records a + completed attempt, not an in-flight request; a frozen value alone cannot + distinguish a blocked request from polling that has not run. + +| Existing evidence | What it distinguishes and what to check next | +|---|---| +| `lastAttemptAt` advances, but `lastSuccessAt` and `observed.collectedAt` do not; failures increase | Polling returns failures. Use the actual error category and method to distinguish TLS, authentication, permissions and collection errors; a stale badge alone does not distinguish them. | +| `observed.collectedAt` advances, but some usage or History panels stay empty | Inventory is refreshing, not necessarily every metric. Follow the [missing-telemetry checks](#inventory-works-but-cpu-memory-or-history-is-missing); do not replace a working key to populate a chart. | +| No timestamps advance, or no completed attempt is recorded | Check that the connection and integration are enabled, then preserve the bounded local error above. This is not proof of an invalid key. | +| Reconnects or TrueNAS sign-ins increase while `observed.collectedAt` advances | Session turnover alone does not establish failed authentication or stopped polling. Record the timing and any existing close/error message separately from the data freshness. | + +**Test Connection** checks connectivity and authentication, not a complete +poll. Testing a saved connection can update `lastAttemptAt` and `lastSuccessAt` +and reset its failure count without refreshing `observed.collectedAt`. Record +any manual test and its time separately; a successful test is not evidence +that missing data or stopped polling has recovered. + +An `app.stats` error such as **Apps are not available** concerns Apps +collection, not necessarily the key. Check the existing Apps status in +TrueNAS, but do not stop Apps or reproduce a hang to gather evidence. Preserve +the existing error before changing anything. Do not repeatedly restart Pulse, +clear History, rotate keys or disable TLS verification as a diagnostic shortcut. + +For a report, include the running Pulse and TrueNAS versions, configured poll +interval, affected panels, before/after timestamps and a reviewed, bounded +error excerpt from the local logs above. Remove private names and addresses +from errors. Do not post the full connection response, credential files, +authentication messages, cookies or a full browser network capture. If safe +inspection is unavailable, describe what you could observe without repeating +the failure or collecting a larger dump. ### Disabling TrueNAS integration Set `PULSE_ENABLE_TRUENAS=false` and restart Pulse. Existing connection data is preserved but polling stops. diff --git a/scripts/tests/test_truenas_docs.py b/scripts/tests/test_truenas_docs.py index 09441b33d3..385edb1a19 100644 --- a/scripts/tests/test_truenas_docs.py +++ b/scripts/tests/test_truenas_docs.py @@ -89,6 +89,57 @@ class TrueNASDocsTest(unittest.TestCase): self.assertIn(required, text) self.assertNotIn("```", text) + def test_polling_guidance_preserves_evidence_before_a_manual_test(self): + text = DOC.read_text().split("### Stale TrueNAS data", 1)[1].split("### Disabling", 1)[0] + for required in ( + "Record the existing state before testing or restarting", + "signed-in Pulse admin browser session", + "`/api/truenas/connections`", "read-only request", + "`poll.intervalSeconds`", "`poll.lastAttemptAt`", + "`poll.lastSuccessAt`", "`poll.consecutiveFailures`", "`poll.lastError`", + "`observed.collectedAt`", "`transport.reconnects`", + "two configured polling cycles", "completed attempt, not an in-flight request", + "without\n pressing **Test Connection**", + "without refreshing `observed.collectedAt`", + "a successful test is not evidence", + "Session turnover alone does not establish failed authentication", + "do not stop Apps or reproduce a hang", + "Do not post the full connection response", + ): + with self.subTest(required=required): + self.assertIn(required, text) + self.assertNotIn("```", text) + self.assertNotIn("Check TrueNAS connectivity and API key validity", text) + # Every suggested field belongs to the existing read-only projection, + # not a made-up diagnostic endpoint or a field in the secret payload. + schemas = { + "poll": (ROOT / "internal/monitoring/truenas_poller.go").read_text().split( + "type TrueNASConnectionPollStatus struct {", 1)[1].split("\n}", 1)[0], + "observed": (ROOT / "internal/monitoring/truenas_poller.go").read_text().split( + "type TrueNASConnectionObservedSummary struct {", 1)[1].split("\n}", 1)[0], + "transport": (ROOT / "internal/truenas/transport.go").read_text().split( + "type TransportStatus struct {", 1)[1].split("\n}", 1)[0], + } + fields = re.findall(r"`(poll|observed|transport)\.([A-Za-z]+)`", text) + self.assertGreater(len(fields), 8) + for projection, field in fields: + with self.subTest(field=f"{projection}.{field}"): + self.assertRegex(schemas[projection], rf'json:"{field}(?:,omitempty)?"') + + def test_general_help_links_to_polling_checks_and_uses_the_actual_default(self): + source = (ROOT / "internal/config/truenas.go").read_text() + default = re.search(r"const defaultTrueNASPollIntervalSecs = (\d+)", source).group(1) + general = (ROOT / "docs/TROUBLESHOOTING.md").read_text() + self.assertEqual((ROOT / "docs/TROUBLESHOOTING.md").read_bytes(), + (ROOT / "frontend-modern/public/docs/TROUBLESHOOTING.md").read_bytes()) + for text in (DOC.read_text(), general): + self.assertIn(f"{default} seconds by default", text) + self.assertIn("TRUENAS.md#stale-truenas-data", general) + self.assertIn("before testing or restarting", general) + self.assertIn("a successful connection test is not proof", general) + self.assertNotIn("(~30 seconds)", DOC.read_text()) + self.assertNotIn("cycle (30s)", general) + def test_preparation_protects_new_and_existing_payload(self): preparation, _ = commands() with tempfile.TemporaryDirectory() as temporary: From d813c936e464eb09bb9ae7e6c7ac33716acde378 Mon Sep 17 00:00:00 2001 From: "pulse-triage[bot]" <249995291+pulse-triage[bot]@users.noreply.github.com> Date: Sat, 3 Oct 2026 00:33:08 +0100 Subject: [PATCH 2/2] Retire alert flapping episodes when cleanup expires their hold Both cleanup owners removed only the deadline, leaving a latched episode that could suppress notifications without a new cooldown. Share dispatch expiry with cleanup, preserve unexpired policies and active occurrence identity, and verify renewed delivery, bounded rearming, diagnosis and one-shot callbacks without bypassing ordinary repeat-notification cooldown. Change-source: pulse-maintainer --- .../v6/internal/subsystems/alerts.md | 12 ++ internal/alerts/active_cleanup.go | 6 +- internal/alerts/flapping_threshold_test.go | 161 ++++++++++++++++++ internal/alerts/notification_policy.go | 22 ++- internal/alerts/tracking_cleanup.go | 5 +- 5 files changed, 193 insertions(+), 13 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/alerts.md b/docs/release-control/v6/internal/subsystems/alerts.md index 3cc8f23219..4b002b9e4b 100644 --- a/docs/release-control/v6/internal/subsystems/alerts.md +++ b/docs/release-control/v6/internal/subsystems/alerts.md @@ -1500,6 +1500,18 @@ actually enforced at dispatch. `internal/alerts/flapping_threshold_test.go` pins these boundaries through normal configuration updates and notification callbacks, alongside the existing flapping cooldown and one-shot callback tests. +Ordinary retention cleanup and hourly tracking cleanup retire a served +suppression deadline, its flapping latch and its observations atomically under +the manager lock, using the same expiry rule as dispatch. Neither sweep may +leave an active occurrence in unbounded flapping suppression after deleting its +deadline. The next burst starts a fresh observation window and can arm a new +bounded cooldown and one-shot callback. Unexpired cooldowns, other keys' policy +and pending bursts without a deadline remain unchanged; expiry does not clear, +acknowledge or change the identity of an active alert. The cleanup regression +controls in `internal/alerts/flapping_threshold_test.go` exercise both sweeps, +drained and retained windows, dispatch callbacks and delivery diagnosis. These +modeled-time controls are not installed notification-destination acceptance. + ### Monitor-only delivery is terminal Monitor-only alerts remain visible, but neither a firing nor a recovery diff --git a/internal/alerts/active_cleanup.go b/internal/alerts/active_cleanup.go index ccf3040e4b..9d0a19af68 100644 --- a/internal/alerts/active_cleanup.go +++ b/internal/alerts/active_cleanup.go @@ -130,10 +130,8 @@ func (m *Manager) Cleanup(maxAge time.Duration) { } } - for id, suppressUntil := range m.suppressedUntil { - if now.After(suppressUntil) { - delete(m.suppressedUntil, id) - } + for id := range m.suppressedUntil { + m.expireSuppressionNoLock(id, now) } cutoff := now.Add(-1 * time.Hour) diff --git a/internal/alerts/flapping_threshold_test.go b/internal/alerts/flapping_threshold_test.go index 25872dca0e..bd75698a5a 100644 --- a/internal/alerts/flapping_threshold_test.go +++ b/internal/alerts/flapping_threshold_test.go @@ -2,6 +2,8 @@ package alerts import ( "fmt" + "reflect" + "sync/atomic" "testing" "time" ) @@ -152,3 +154,162 @@ func TestFlappingLoweredThresholdBoundsRetainedHistory(t *testing.T) { t.Fatalf("lowered threshold not enforced with bounded history: suppressed=%v transitioned=%v history=%d", suppressed, transitioned, historySize) } } + +// Model elapsed cooldown time, but exercise the real cleanup, dispatch, +// one-shot callback and diagnosis paths. A sweep must not turn a timed hold +// into an indefinitely latched episode, whether its observation window drained +// or still contains the previous burst. +func TestFlappingCleanupReleasesAndRearmsDelivery(t *testing.T) { + for _, sweep := range []struct { + name string + run func(*Manager) + }{ + {"ordinary", func(m *Manager) { m.Cleanup(time.Hour) }}, + {"hourly", (*Manager).cleanupStaleMaps}, + } { + for _, window := range []int{60, 3600} { + t.Run(fmt.Sprintf("%s/window_%d", sweep.name, window), func(t *testing.T) { + m := NewManagerWithDataDir(t.TempDir()) + t.Cleanup(m.Stop) + cfg := m.GetConfig() + cfg.Enabled = true + cfg.ActivationState = ActivationActive + cfg.AutoAcknowledgeAfterHours = 0 + cfg.Schedule.Cooldown = 0 + cfg.Schedule.QuietHours.Enabled = false + cfg.FlappingEnabled = true + cfg.FlappingThreshold = 3 + cfg.FlappingWindowSeconds = window + cfg.FlappingCooldownMinutes = 15 + m.UpdateConfig(cfg) + + _, alert := testNewCanonicalAlert("vm-cleanup", "vm-cleanup-cpu", "vm", "cpu") + alert.Level = AlertLevelWarning + alert.StartTime = time.Now() + alert.LastSeen = alert.StartTime + m.mu.Lock() + m.setActiveAlertNoLock(alert.ID, alert) + m.mu.Unlock() + key := canonicalTrackingKeyForAlert(alert) + var deliveries atomic.Int32 + m.SetAlertCallback(func(*Alert) { deliveries.Add(1) }) + transitions := make(chan string, 4) + m.SetFlappingDetectedCallback(func(a *Alert, trackingKey string) { + // Re-enter the manager: the one-shot callback must remain outside its lock. + active := m.GetActiveAlerts() + if len(active) != 1 || a.ID != alert.ID || trackingKey != key { + t.Error("flapping callback lost the continuing occurrence identity") + } + transitions <- trackingKey + }) + dispatch := func() bool { + m.mu.Lock() + defer m.mu.Unlock() + return m.dispatchAlert(alert, false) + } + receiveTransition := func() { + t.Helper() + select { + case <-transitions: + case <-time.After(2 * time.Second): + t.Error("new flapping episode did not notify its one-shot callback") + } + } + for attempt := 1; attempt <= cfg.FlappingThreshold; attempt++ { + if got, want := dispatch(), attempt < cfg.FlappingThreshold; got != want { + t.Fatalf("initial dispatch %d = %v, want %v", attempt, got, want) + } + } + receiveTransition() + + m.mu.Lock() + for i := range m.flappingHistory[key] { + m.flappingHistory[key][i] = m.flappingHistory[key][i].Add(-16 * time.Minute) + } + m.suppressedUntil[key] = time.Now().Add(-time.Second) + m.mu.Unlock() + sweep.run(m) + diagnosis, found := m.DiagnoseAlertDelivery(alert.ID) + if !found || diagnosis.FlappingActive || diagnosis.FlappingHistoryInWindow != 0 || + diagnosis.SuppressedUntil != nil || diagnosis.Reason != AlertDeliveryReasonCooldown { + t.Errorf("served episode not retired coherently: %+v", diagnosis) + } + for attempt := 1; attempt <= cfg.FlappingThreshold; attempt++ { + if got, want := dispatch(), attempt < cfg.FlappingThreshold; got != want { + t.Errorf("post-cleanup dispatch %d = %v, want %v", attempt, got, want) + } + } + receiveTransition() + diagnosis, _ = m.DiagnoseAlertDelivery(alert.ID) + if diagnosis.Reason != AlertDeliveryReasonFlapping || !diagnosis.FlappingActive || + diagnosis.SuppressedUntil == nil || !diagnosis.SuppressedUntil.After(time.Now()) { + t.Errorf("new storm has no bounded cooldown: %+v", diagnosis) + } + deadline := diagnosis.SuppressedUntil + for range 20 { + sweep.run(m) + if dispatch() { + t.Error("cleanup released an unexpired cooldown") + } + } + final, _ := m.DiagnoseAlertDelivery(alert.ID) + if !reflect.DeepEqual(final.SuppressedUntil, deadline) || final.FlappingHistoryInWindow != 3 || + deliveries.Load() != 4 || len(transitions) != 0 { + t.Errorf("cooldown changed during cleanup: before=%+v after=%+v deliveries=%d extra callbacks=%d", + diagnosis, final, deliveries.Load(), len(transitions)) + } + active := m.GetActiveAlerts() + if len(active) != 1 || active[0].ID != alert.ID || !active[0].StartTime.Equal(alert.StartTime) || active[0].Acknowledged { + t.Fatal("suppression expiry changed or removed the still-active occurrence") + } + t.Logf("two bounded episodes: %d dispatch callbacks; both cleanup and delivery diagnosis preserve occurrence %s", deliveries.Load(), alert.ID) + }) + } + } +} + +func TestFlappingCleanupKeepsUnexpiredAndOtherKeys(t *testing.T) { + for _, sweep := range []struct { + name string + run func(*Manager) + }{ + {"ordinary", func(m *Manager) { m.Cleanup(time.Hour) }}, + {"hourly", (*Manager).cleanupStaleMaps}, + } { + t.Run(sweep.name, func(t *testing.T) { + m := NewManagerWithDataDir(t.TempDir()) + t.Cleanup(m.Stop) + now := time.Now() + m.mu.Lock() + m.config.AutoAcknowledgeAfterHours = 0 + for _, key := range []string{"expired-episode", "ongoing-episode", "ordinary-suppression", "pending-burst"} { + m.activeAlerts[key] = &Alert{ID: key, StartTime: now, LastSeen: now} + m.flappingHistory[key] = []time.Time{now} + } + m.flappingActive["expired-episode"] = true + m.flappingActive["ongoing-episode"] = true + m.suppressedUntil["expired-episode"] = now.Add(-time.Second) + m.suppressedUntil["ongoing-episode"] = now.Add(time.Hour) + m.suppressedUntil["ordinary-suppression"] = now.Add(time.Hour) + m.mu.Unlock() + sweep.run(m) + m.mu.RLock() + defer m.mu.RUnlock() + if m.flappingActive["expired-episode"] || len(m.flappingHistory["expired-episode"]) != 0 { + t.Error("expired suppression kept its episode") + } + if _, exists := m.suppressedUntil["expired-episode"]; exists { + t.Error("expired deadline retained") + } + if !m.flappingActive["ongoing-episode"] || !m.suppressedUntil["ongoing-episode"].Equal(now.Add(time.Hour)) || + !m.suppressedUntil["ordinary-suppression"].Equal(now.Add(time.Hour)) || m.flappingActive["ordinary-suppression"] { + t.Error("cleanup altered another key's unexpired policy") + } + for _, key := range []string{"ongoing-episode", "ordinary-suppression", "pending-burst"} { + if !reflect.DeepEqual(m.flappingHistory[key], []time.Time{now}) { + t.Errorf("cleanup altered %s observations", key) + } + } + }) + } +} diff --git a/internal/alerts/notification_policy.go b/internal/alerts/notification_policy.go index 4356f95f77..2fc298450e 100644 --- a/internal/alerts/notification_policy.go +++ b/internal/alerts/notification_policy.go @@ -103,12 +103,7 @@ func (m *Manager) checkFlappingLocked(trackingKey string) (suppress bool, justTr if now.Before(until) { return true, false } - // The cooldown has been served. Clear the latch so a later episode can - // open a fresh one; flappingActive is otherwise only ever set to true, - // which would leave every subsequent episode with no cooldown at all. - delete(m.suppressedUntil, trackingKey) - delete(m.flappingActive, trackingKey) - delete(m.flappingHistory, trackingKey) + m.expireSuppressionNoLock(trackingKey, now) } // Record this state change @@ -162,6 +157,21 @@ func (m *Manager) checkFlappingLocked(trackingKey string) (suppress bool, justTr return false, false } +// expireSuppressionNoLock retires a served suppression and its episode together. +// Cleanup must use the same reset as dispatch: deleting only the deadline leaves +// a latched flapping episode that can suppress delivery without ever rearming a +// cooldown or notifying the flapping callback. Caller MUST hold m.mu. +func (m *Manager) expireSuppressionNoLock(trackingKey string, now time.Time) bool { + until, exists := m.suppressedUntil[trackingKey] + if !exists || now.Before(until) { + return false + } + delete(m.suppressedUntil, trackingKey) + delete(m.flappingActive, trackingKey) + delete(m.flappingHistory, trackingKey) + return true +} + func (m *Manager) dispatchAlert(alert *Alert, async bool) bool { callbacks := m.getAlertCallbacks() if len(callbacks) == 0 || alert == nil { diff --git a/internal/alerts/tracking_cleanup.go b/internal/alerts/tracking_cleanup.go index 7b7294b6d8..a328600064 100644 --- a/internal/alerts/tracking_cleanup.go +++ b/internal/alerts/tracking_cleanup.go @@ -45,9 +45,8 @@ func (m *Manager) cleanupStaleMaps() { } } - for alertID, suppressUntil := range m.suppressedUntil { - if now.After(suppressUntil) { - delete(m.suppressedUntil, alertID) + for alertID := range m.suppressedUntil { + if m.expireSuppressionNoLock(alertID, now) { cleaned++ } }