fix(monitor): a permanent dismissal stays permanent, and every host backup run is notified

AppImage/scripts/health_persistence.py

@@ -791,6 +791,7 @@ class HealthPersistence:
                     resolution_type = COALESCE(resolution_type, 'auto'),
                     resolution_reason = ?
                 WHERE error_key = ? AND resolved_at IS NULL
+                  AND NOT (acknowledged = 1 AND IFNULL(suppression_hours, 0) = -1)
             ''', (now, reason, error_key))
 
             if cursor.rowcount > 0:
@@ -904,10 +905,14 @@ class HealthPersistence:
 
             # Check if this error was acknowledged (dismissed)
             cursor.execute('''
-                SELECT acknowledged FROM errors WHERE error_key = ?
+                SELECT acknowledged, suppression_hours FROM errors WHERE error_key = ?
             ''', (error_key,))
             row = cursor.fetchone()
 
+            if row and row[0] == 1 and row[1] == -1:
+                # Silenced permanently: it stays silenced if the condition
+                # comes back, until the user re-enables it.
+                return
             if row and row[0] == 1:
                 # Dismissed error that naturally resolved - delete entirely
                 cursor.execute('DELETE FROM errors WHERE error_key = ?', (error_key,))
@@ -1297,6 +1302,13 @@ class HealthPersistence:
             now = datetime.now()
             now_iso = now.isoformat()
 
+            # A permanent dismissal that was closed only for its age is kept.
+            cursor.execute(
+                'UPDATE errors SET resolved_at = NULL, resolution_type = NULL, resolution_reason = NULL '
+                'WHERE acknowledged = 1 AND suppression_hours = -1 AND resolution_reason IN (%s)'
+                % ','.join('?' * len(self._AGED_OUT_REASONS)),
+                tuple(sorted(self._AGED_OUT_REASONS)))
+
             # Delete resolved errors older than 7 days
             cutoff_resolved = (now - timedelta(days=7)).isoformat()
             cursor.execute('DELETE FROM errors WHERE resolved_at < ?', (cutoff_resolved,))
@@ -1463,6 +1475,14 @@ class HealthPersistence:
         # NOTE: nothing here touches `disk_observations`. That table is the
         # permanent history of each disk and no cycle removes or hides rows.
     
+    # Why the stale cleanup closes a record when nothing else did: its age.
+    _AGED_OUT_REASONS = frozenset({
+        'Log error aged out (>48h)',
+        'Temperature error stale (>24h no activity)',
+        'Update/security notice aged out (>7d)',
+        'Stale error (no activity >7d)',
+    })
+
     def _cleanup_stale_resources(self):
         """Resolve errors for resources that no longer exist.
         
@@ -1490,7 +1510,8 @@ class HealthPersistence:
         # An error is considered unresolved if resolution_type is NULL or empty
         # (resolved_at alone is not sufficient - it may be in an inconsistent state)
         cursor.execute('''
-            SELECT id, error_key, category, reason, first_seen, last_seen, severity FROM errors 
+            SELECT id, error_key, category, reason, first_seen, last_seen, severity,
+                   acknowledged, suppression_hours FROM errors
             WHERE resolution_type IS NULL OR resolution_type = ''
         ''')
         active_errors = cursor.fetchall()
@@ -1694,7 +1715,9 @@ class HealthPersistence:
                 return 0
         
         for error_row in active_errors:
-            err_id, error_key, category, reason, first_seen, last_seen, severity = error_row
+            (err_id, error_key, category, reason, first_seen, last_seen, severity,
+             acknowledged, suppression_hours) = error_row
+            permanent = acknowledged == 1 and suppression_hours == -1
             should_resolve = False
             resolution_reason = None
             age_hours = get_age_hours(first_seen)
@@ -1899,6 +1922,12 @@ class HealthPersistence:
                 should_resolve = True
                 resolution_reason = 'Stale error (no activity >7d)'
             
+            # A problem silenced permanently is not recorded again on purpose,
+            # so its lack of activity says nothing: only its resource going
+            # away ends it. The user brings it back from Active Suppressions.
+            if should_resolve and permanent and resolution_reason in self._AGED_OUT_REASONS:
+                should_resolve = False
+
             if should_resolve:
                 cursor.execute('''
                     UPDATE errors SET resolved_at = ?, resolution_type = 'auto', resolution_reason = ?
@@ -2348,6 +2377,10 @@ class HealthPersistence:
         
         updated_count = 0
         for err_id, error_key, category, old_hours in dismissed:
+            if old_hours == -1:
+                # A permanent dismissal is the user's choice for that alert,
+                # and only ends from Active Suppressions.
+                continue
             setting_key = None
             for skey, meta in self._get_category_labels().items():
                 if meta['category'] == category:

AppImage/scripts/notification_manager.py

@@ -1618,6 +1618,7 @@ class NotificationManager:
     # opt-in. Mirrors the same lists from _passes_cooldown.
     _DIGEST_EXEMPT_EVENTS = frozenset({
         'backup_complete', 'backup_fail', 'backup_start',
+        'host_backup_complete', 'host_backup_fail', 'host_backup_start',
         'replication_complete', 'replication_fail',
         'vm_start', 'vm_stop', 'vm_shutdown', 'vm_restart',
         'ct_start', 'ct_stop', 'ct_shutdown', 'ct_restart',
@@ -2125,6 +2126,7 @@ class NotificationManager:
         # always be delivered. A 10s cooldown prevents exact duplicates
         # (webhook + tasks) but allows repeated backup jobs to report.
         _ALWAYS_DELIVER = {'backup_complete', 'backup_fail', 'backup_start',
+                           'host_backup_complete', 'host_backup_fail', 'host_backup_start',
                            'replication_complete', 'replication_fail'}
         if event.event_type in _ALWAYS_DELIVER and cooldown_str is None:
             cooldown = 10

AppImage/scripts/tests/test_host_backup_notifications_every_run.py

@@ -0,0 +1,44 @@
+"""Every run of a host backup is reported, the way a VM or CT backup is: a
+nightly job is not the same event as the night before, and its result is not
+kept for the daily digest."""
+import sys
+from pathlib import Path
+import unittest
+
+sys.path.insert(0, str(Path(__file__).resolve().parents[1]))
+import notification_manager  # noqa: E402
+from notification_events import NotificationEvent  # noqa: E402
+
+BACKUP_EVENTS = ('backup_start', 'backup_complete', 'backup_fail',
+                 'host_backup_start', 'host_backup_complete', 'host_backup_fail')
+
+
+class HostBackupEveryRunTests(unittest.TestCase):
+    def setUp(self):
+        self.manager = notification_manager.NotificationManager.__new__(notification_manager.NotificationManager)
+        self.manager._config = {'email.digest_enabled': 'true'}
+
+    def event(self, event_type, severity='INFO'):
+        return NotificationEvent(event_type, severity, {'hostname': 'pve'}, source='test', entity='node', entity_id='')
+
+    def test_two_runs_a_day_apart_or_closer_are_both_sent(self):
+        for event_type in BACKUP_EVENTS:
+            with self.subTest(event_type=event_type):
+                severity = 'CRITICAL' if event_type.endswith('_fail') else 'INFO'
+                self.assertEqual(self.manager._cooldown_seconds(self.event(event_type, severity)), 10)
+
+    def test_the_result_is_not_kept_for_the_daily_digest(self):
+        for event_type in BACKUP_EVENTS:
+            with self.subTest(event_type=event_type):
+                self.assertFalse(self.manager._should_buffer_for_digest('email', 'INFO', event_type))
+
+    def test_a_cooldown_set_by_the_user_is_still_respected(self):
+        self.manager._config['cooldown.host_backup_complete'] = '3600'
+        self.assertEqual(self.manager._cooldown_seconds(self.event('host_backup_complete')), 3600)
+
+    def test_other_informational_events_still_go_to_the_digest(self):
+        self.assertTrue(self.manager._should_buffer_for_digest('email', 'INFO', 'update_summary'))
+
+
+if __name__ == '__main__':
+    unittest.main()

AppImage/scripts/tests/test_permanent_dismissal_is_kept.py

@@ -0,0 +1,108 @@
+"""An alert dismissed permanently stays dismissed until the user re-enables
+it: not seeing it for a while, the condition recovering or a change of the
+suppression settings does not bring it back."""
+import sqlite3
+import sys
+import tempfile
+import unittest
+from datetime import datetime, timedelta
+from pathlib import Path
+from unittest.mock import patch
+
+SCRIPTS = Path(__file__).resolve().parents[1]
+sys.path.insert(0, str(SCRIPTS))
+# The module opens the database of the host when it is imported.
+_connect = sqlite3.connect
+with patch.object(Path, 'mkdir'), patch('builtins.print'), \
+        patch('sqlite3.connect', side_effect=lambda *args, **kwargs: _connect(':memory:')):
+    import health_persistence as hp_module
+
+KEY = 'pve_storage_full_local-lvm'
+
+
+class PermanentDismissalTests(unittest.TestCase):
+    def setUp(self):
+        tmp = tempfile.TemporaryDirectory()
+        self.addCleanup(tmp.cleanup)
+        store = hp_module.HealthPersistence.__new__(hp_module.HealthPersistence)
+        for name, value in vars(hp_module.health_persistence).items():
+            setattr(store, name, value)
+        store.data_dir = Path(tmp.name)
+        store.db_path = store.data_dir / 'health_monitor.db'
+        with patch('builtins.print'):
+            store._init_database()
+        self.store = store
+
+    def seen(self):
+        return self.store.record_error(KEY, 'storage', 'WARNING', 'PVE storage local-lvm (lvmthin) at 96.0% used',
+                                       {'storage_name': 'local-lvm'})
+
+    def days_pass(self, days):
+        """Moves every date of the record back, as if that time had gone by."""
+        conn = self.store._get_conn()
+        row = conn.execute('SELECT first_seen, last_seen, acknowledged_at, resolved_at FROM errors WHERE error_key = ?',
+                           (KEY,)).fetchone()
+        if row:
+            moved = [(datetime.fromisoformat(v) - timedelta(days=days)).isoformat() if v else None for v in row]
+            conn.execute('UPDATE errors SET first_seen = ?, last_seen = ?, acknowledged_at = ?, resolved_at = ? '
+                         'WHERE error_key = ?', (*moved, KEY))
+            conn.commit()
+        conn.close()
+
+    def cleanup(self):
+        with patch('builtins.print'), patch('subprocess.run', side_effect=OSError('no host tools in the test')):
+            self.store.cleanup_old_errors()
+
+    def test_it_is_still_dismissed_weeks_later(self):
+        self.assertTrue(self.seen()['needs_notification'])
+        self.store.acknowledge_error(KEY, suppression_hours=-1)
+        for _ in range(6):
+            self.days_pass(8)
+            self.cleanup()
+            self.assertFalse(self.seen()['needs_notification'])
+        self.assertTrue(self.store.is_error_permanently_acknowledged(KEY))
+
+    def test_a_condition_that_recovers_and_returns_stays_dismissed(self):
+        self.seen()
+        self.store.acknowledge_error(KEY, suppression_hours=-1)
+        self.store.clear_error(KEY)
+        self.store.resolve_error(KEY, 'condition recovered')
+        self.days_pass(8)
+        self.cleanup()
+        self.assertFalse(self.seen()['needs_notification'])
+        self.assertTrue(self.store.is_error_permanently_acknowledged(KEY))
+
+    def test_a_record_already_closed_for_its_age_is_kept(self):
+        self.seen()
+        self.store.acknowledge_error(KEY, suppression_hours=-1)
+        conn = self.store._get_conn()
+        closed = (datetime.now() - timedelta(days=8)).isoformat()
+        conn.execute("UPDATE errors SET resolved_at = ?, resolution_type = 'auto', "
+                     "resolution_reason = 'Stale error (no activity >7d)' WHERE error_key = ?", (closed, KEY))
+        conn.commit()
+        conn.close()
+        self.cleanup()
+        self.assertFalse(self.seen()['needs_notification'])
+
+    def test_saving_the_suppression_settings_leaves_it_permanent(self):
+        self.seen()
+        self.store.acknowledge_error(KEY, suppression_hours=-1)
+        with patch('builtins.print'):
+            self.store.sync_dismissed_suppression()
+        self.assertTrue(self.store.is_error_permanently_acknowledged(KEY))
+
+    def test_re_enabling_it_brings_the_alert_back(self):
+        self.seen()
+        self.store.acknowledge_error(KEY, suppression_hours=-1)
+        self.store.unacknowledge_error(KEY)
+        self.assertFalse(self.store.is_error_permanently_acknowledged(KEY))
+
+    def test_a_timed_dismissal_still_ends(self):
+        self.seen()
+        self.store.acknowledge_error(KEY, suppression_hours=24)
+        self.days_pass(2)
+        self.assertTrue(self.seen()['needs_notification'])
+
+
+if __name__ == '__main__':
+    unittest.main()