fix(monitor): do not report a service reloading or restarting as failed

AppImage/scripts/health_monitor.py

@@ -385,6 +385,10 @@ class HealthMonitor:
     
     # PVE Critical Services
     PVE_SERVICES = ['pveproxy', 'pvedaemon', 'pvestatd', 'pve-cluster']
+    # A restart passes through these for a few seconds; a service that stays
+    # in one of them longer is not restarting, it is failing to.
+    SERVICE_TRANSIENT_STATES = ('activating', 'deactivating')
+    SERVICE_SETTLE_SECONDS = 8
     
     # P2 fix: Pre-compiled regex patterns for performance (avoid re-compiling on every line)
     _BENIGN_RE = None
@@ -3893,6 +3897,28 @@ class HealthMonitor:
             print(f"[HealthMonitor] VMs/CTs persistence check failed: {e}")
             return {'status': 'UNKNOWN', 'reason': f'VM/CT check unavailable: {str(e)}', 'checks': {}, 'dismissable': True}
     
+    def _service_state(self, service: str) -> str:
+        """The state of a service, once a restart in progress has finished."""
+        deadline = time.time() + self.SERVICE_SETTLE_SECONDS
+        while True:
+            state = subprocess.run(
+                ['systemctl', 'is-active', service],
+                capture_output=True,
+                text=True,
+                timeout=2
+            ).stdout.strip()
+            if state not in self.SERVICE_TRANSIENT_STATES or time.time() >= deadline:
+                return state
+            time.sleep(1)
+
+    def _package_update_running(self) -> bool:
+        """An update stops and restarts the services it replaces."""
+        try:
+            from notification_events import is_apt_active_on_host
+            return is_apt_active_on_host()
+        except Exception:
+            return False
+
     def _check_pve_services(self) -> Dict[str, Any]:
         """
         Check critical Proxmox services with persistence tracking.
@@ -3923,21 +3949,21 @@ class HealthMonitor:
 
             for service in services_to_check:
                 try:
-                    result = subprocess.run(
-                        ['systemctl', 'is-active', service],
-                        capture_output=True,
-                        text=True,
-                        timeout=2
-                    )
-
-                    status = result.stdout.strip()
-                    if result.returncode != 0 or status != 'active':
+                    status = self._service_state(service)
+                    # A service reloading its configuration keeps running.
+                    if status not in ('active', 'reloading'):
                         failed_services.append(service)
                         service_details[service] = status or 'inactive'
                 except Exception:
                     failed_services.append(service)
                     service_details[service] = 'error'
 
+            # During a package update nothing is concluded about these services
+            # in either direction: the next check after it does that.
+            in_update = []
+            if failed_services and self._package_update_running():
+                in_update, failed_services = failed_services, []
+
             # Auto-clear errors for services that are healthy this cycle.
             # Lifted out of the `if failed_services:` branch so it runs
             # unconditionally — otherwise a service that recovered while
@@ -3945,7 +3971,7 @@ class HealthMonitor:
             # active forever (the dismissed-only path returned before the
             # cleanup loop). Audit Tier 5 — Health stack.
             for svc in services_to_check:
-                if svc not in failed_services:
+                if svc not in failed_services and svc not in in_update:
                     error_key = f'{service_prefix}_service_{svc}'
                     if health_persistence.is_error_active(error_key):
                         health_persistence.clear_error(error_key)
@@ -3962,6 +3988,12 @@ class HealthMonitor:
                         'error_key': error_key,
                         'dismissable': True,
                     }
+                elif svc in in_update:
+                    checks[svc] = {
+                        'status': 'OK',
+                        'detail': f"Service is {service_details.get(svc, 'inactive')}",
+                        'error_key': error_key,
+                    }
                 else:
                     checks[svc] = {
                         'status': 'OK',

AppImage/scripts/tests/test_pbs.py

@@ -274,14 +274,17 @@ class PBSManagerTests(unittest.TestCase):
         names = ("_check_pve_services", "_check_vms_cts_optimized", "_check_vms_cts_with_persistence",
                  "_check_proxmox_storages", "_check_proxmox_storage", "_check_lxc_disk_usage",
                  "_check_vm_disk_usage", "_check_lxc_mount_capacity", "_check_pve_storage_capacity")
-        functions = [node for node in ast.walk(source) if isinstance(node, ast.FunctionDef) and node.name in names]
+        functions = [node for node in ast.walk(source) if isinstance(node, ast.FunctionDef)
+                     and node.name in (*names, "_service_state")]
         persistence = mock.Mock(); persistence.is_error_active.return_value = False
         run = mock.Mock(return_value=SimpleNamespace(returncode=0, stdout="active\n"))
         namespace = {"Dict": dict, "Any": object, "Optional": __import__('typing').Optional,
                      "PBS_SERVICES": pbs.PBS_SERVICES, "health_persistence": persistence,
-                     "subprocess": SimpleNamespace(run=run), "os": __import__('os')}
+                     "subprocess": SimpleNamespace(run=run), "os": __import__('os'), "time": __import__('time')}
         exec(compile(ast.Module(body=functions, type_ignores=[]), '<health-pbs-test>', 'exec'), namespace)
         instance = SimpleNamespace(platform_type="pbs", PVE_SERVICES=["pveproxy", "pvedaemon", "pvestatd", "pve-cluster"])
+        instance.SERVICE_TRANSIENT_STATES, instance.SERVICE_SETTLE_SECONDS = (), 0
+        instance._service_state = lambda service: namespace["_service_state"](instance, service)
         services = namespace["_check_pve_services"](instance)
         self.assertEqual(services["status"], "OK")
         self.assertEqual([call.args[0][2] for call in run.call_args_list], list(pbs.PBS_SERVICES))

AppImage/scripts/tests/test_pve_service_states.py

@@ -0,0 +1,84 @@
+"""A service reloading or restarting is working; a failed one is still reported."""
+import sys
+import tempfile
+import unittest
+from pathlib import Path
+from types import SimpleNamespace
+from unittest.mock import patch
+
+SCRIPTS_DIR = Path(__file__).resolve().parents[1]
+if str(SCRIPTS_DIR) not in sys.path:
+    sys.path.insert(0, str(SCRIPTS_DIR))
+TESTS_DIR = str(Path(__file__).resolve().parent)
+if TESTS_DIR not in sys.path:
+    sys.path.insert(0, TESTS_DIR)
+
+from health_fixture_modules import load_health_modules
+
+
+class ServiceStates(unittest.TestCase):
+    def setUp(self):
+        folder = tempfile.TemporaryDirectory()
+        self.addCleanup(folder.cleanup)
+        self.module, _, _ = load_health_modules(folder.name)
+        self.monitor = self.module.HealthMonitor.__new__(self.module.HealthMonitor)
+        self.persistence = self.module.health_persistence
+
+    def check(self, states, updating=False):
+        """`states` gives each service its answers to is-active, one per call."""
+        answers = {service: list(values) for service, values in states.items()}
+
+        def is_active(command, **_kwargs):
+            pending = answers.get(command[-1], ['active'])
+            return SimpleNamespace(stdout=(pending.pop(0) if len(pending) > 1 else pending[0]) + '\n')
+
+        with patch.object(self.module.subprocess, 'run', side_effect=is_active), \
+                patch.object(self.module.time, 'sleep'), \
+                patch.object(self.monitor, '_package_update_running', return_value=updating):
+            return self.monitor._check_pve_services()
+
+    def active(self, service):
+        return self.persistence.is_error_active(f'pve_service_{service}')
+
+    def test_a_service_reloading_is_not_a_failure(self):
+        result = self.check({'pveproxy': ['reloading']})
+        self.assertEqual(result['status'], 'OK')
+        self.assertFalse(self.active('pveproxy'))
+
+    def test_a_restart_that_ends_is_not_a_failure(self):
+        result = self.check({'pveproxy': ['deactivating', 'activating', 'active']})
+        self.assertEqual(result['status'], 'OK')
+        self.assertFalse(self.active('pveproxy'))
+
+    def test_a_service_that_never_finishes_starting_is_reported(self):
+        with patch.object(self.module.HealthMonitor, 'SERVICE_SETTLE_SECONDS', 0):
+            result = self.check({'pveproxy': ['activating']})
+        self.assertEqual(result['status'], 'CRITICAL')
+        self.assertTrue(self.active('pveproxy'))
+
+    def test_a_failed_service_is_reported_at_once(self):
+        for state in ('failed', 'inactive'):
+            with self.subTest(state=state):
+                result = self.check({'pvedaemon': [state]})
+                self.assertEqual(result['status'], 'CRITICAL')
+                self.assertEqual(result['failed'], ['pvedaemon'])
+
+    def test_a_package_update_stops_nothing_being_reported(self):
+        result = self.check({'pveproxy': ['inactive']}, updating=True)
+        self.assertEqual(result['status'], 'OK')
+        self.assertEqual(result['checks']['pveproxy']['detail'], 'Service is inactive')
+        self.assertFalse(self.active('pveproxy'))
+
+    def test_a_package_update_does_not_clear_a_real_failure(self):
+        self.check({'pveproxy': ['failed']})
+        self.check({'pveproxy': ['inactive']}, updating=True)
+        self.assertTrue(self.active('pveproxy'))
+
+    def test_the_failure_shows_once_the_update_has_finished(self):
+        self.check({'pveproxy': ['inactive']}, updating=True)
+        result = self.check({'pveproxy': ['inactive']})
+        self.assertEqual(result['status'], 'CRITICAL')
+
+
+if __name__ == '__main__':
+    unittest.main()