fix(oci): keep update results when one application fails

- Check that the Monitor answers on its socket, not only that it exists
- Report the applications whose check failed instead of failing the whole request
- Show those applications as not checked, without their older result
- Tell the user in the menu when a check fails

oci/remote/oci_update_check.py

@@ -95,13 +95,17 @@ def snapshot(root=instances.ROOT, configs=Path('/etc/pve/lxc')):
     return applications
 
 
-def cached(root=instances.ROOT, configs=Path('/etc/pve/lxc'), now=None, apps=APPS):
-    """Read the Monitor's sidecars; obey its primary-container stack policy."""
+def cached(root=instances.ROOT, configs=Path('/etc/pve/lxc'), now=None, apps=APPS, failed=()):
+    """Read the Monitor's sidecars; obey its primary-container stack policy.
+    An application whose last check failed keeps no older result."""
     inventory = snapshot(root, configs)
     result = {'checked': False, 'available': [], 'unknown': 0, 'applications': len(inventory)}
     instant = time.time() if now is None else now
     for app in inventory:
         try:
+            if app['vmid'] in failed:
+                result['checked'] = True
+                raise ValueError('Check failed')
             if not app['complete'] or not all(member['ready'] for member in app['members']):
                 raise ValueError('Incomplete installation')
             main = next(member for member in app['members'] if member['vmid'] == app['vmid'])
@@ -134,9 +138,15 @@ def cached(root=instances.ROOT, configs=Path('/etc/pve/lxc'), now=None, apps=APP
 def monitor_available(path=SOCKET):
     try:
         parent, endpoint = path.parent.lstat(), path.lstat()
-        return (stat.S_ISDIR(parent.st_mode) and parent.st_uid == 0
+        if not (stat.S_ISDIR(parent.st_mode) and parent.st_uid == 0
                 and not parent.st_mode & 0o077 and stat.S_ISSOCK(endpoint.st_mode)
-                and endpoint.st_uid == 0 and not endpoint.st_mode & 0o077)
+                and endpoint.st_uid == 0 and not endpoint.st_mode & 0o077):
+            return False
+        # A Monitor that stopped without cleaning up leaves the socket behind.
+        with socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) as probe:
+            probe.settimeout(0.5)
+            probe.connect(str(path))
+        return True
     except OSError:
         return False
 
@@ -167,7 +177,10 @@ def check(root=instances.ROOT, configs=Path('/etc/pve/lxc'), progress=None, path
                     raise RuntimeError('Monitor check incomplete')
                 event = json.loads(line)
                 if event.get('done') is True and completed == len(rows):
-                    return
+                    failed = event.get('failed', [])
+                    if not isinstance(failed, list) or any(type(vmid) is not int for vmid in failed):
+                        raise RuntimeError('Monitor check incomplete')
+                    return failed
                 done, count = event.get('completed'), event.get('total')
                 if (type(done) is not int or type(count) is not int
                         or count != len(rows) or not completed <= done <= count or done < 0):
@@ -177,14 +190,18 @@ def check(root=instances.ROOT, configs=Path('/etc/pve/lxc'), progress=None, path
                     progress(skipped + completed, total)
 
 
-def emit_progress(completed, total):
-    message = (json.dumps({'completed': completed, 'total': total}) + '\n').encode()
+def emit(event):
+    message = (json.dumps(event) + '\n').encode()
     try:
         os.write(sys.stdout.fileno(), message)
     except (BlockingIOError, BrokenPipeError):
         pass
 
 
+def emit_progress(completed, total):
+    emit({'completed': completed, 'total': total})
+
+
 def main():
     parser = argparse.ArgumentParser(description=__doc__)
     parser.add_argument('--root', type=Path, default=instances.ROOT)
@@ -194,7 +211,9 @@ def main():
     try:
         if args.progress:
             os.set_blocking(sys.stdout.fileno(), False)
-        check(args.root, args.configs, progress=emit_progress if args.progress else None)
+        failed = check(args.root, args.configs, progress=emit_progress if args.progress else None)
+        if args.progress and failed:
+            emit({'failed': failed})
         return 0
     except (OSError, ValueError, KeyError, TypeError, RuntimeError):
         return 1

oci/src/proxmenux_oci/updates.py

@@ -20,6 +20,7 @@ class UpdateCheck:
         self.backend = importlib.import_module('oci_update_check')
         self.process = None
         self.failed = False
+        self.failed_applications = set()
         self.completed = self.total = 0
         self._progress_buffer = b''
         self._shown_progress = (0, 0)
@@ -52,6 +53,9 @@ class UpdateCheck:
                     line, self._progress_buffer = self._progress_buffer.split(b'\n', 1)
                     try:
                         data = json.loads(line)
+                        if isinstance(data.get('failed'), list):
+                            self.failed_applications = {vmid for vmid in data['failed'] if type(vmid) is int}
+                            continue
                         completed, total = data['completed'], data['total']
                         if type(completed) is int and type(total) is int and 0 <= completed <= total:
                             self.completed, self.total = completed, total
@@ -77,7 +81,7 @@ class UpdateCheck:
             self._close_output()
             self.process = None
         try:
-            state = self.backend.cached()
+            state = self.backend.cached(failed=self.failed_applications)
         except (OSError, ValueError, KeyError, TypeError):
             state = {'checked': False, 'available': [], 'unknown': 0, 'applications': 0}
         if self.failed:
@@ -93,6 +97,7 @@ class UpdateCheck:
             raise ValueError(translate('The ProxMenux Monitor update checker is not available. Start or update ProxMenux Monitor and try again.'))
         state = self.status()
         self.failed = False
+        self.failed_applications = set()
         self.completed, self.total = 0, state['applications']
         self._progress_buffer = b''
         self.process = subprocess.Popen(
@@ -142,7 +147,14 @@ class UpdateCheck:
         return f"{translate('Updates available')} ({count})" if count else translate('Check for updates')
 
     def notice(self, state):
-        return ''  # Keep the menu heading clear after a check.
+        if self.active:
+            return ''
+        if self.failed:
+            return translate('The update check could not be completed. Try again later.')
+        if self.failed_applications:
+            return translate('Some applications could not be checked ({count}). Try again later.').format(
+                count=len(self.failed_applications))
+        return ''
 
 
 def select_updates(project, ui, available):

oci/tests/test_update_check.py

@@ -177,7 +177,8 @@ class UpdateCheckTests(unittest.TestCase):
     def test_readonly_monitor_availability_rejects_non_private_endpoints(self):
         path = Mock(); path.parent.lstat.return_value = SimpleNamespace(st_mode=stat.S_IFDIR | 0o700, st_uid=0)
         path.lstat.return_value = SimpleNamespace(st_mode=stat.S_IFSOCK | 0o600, st_uid=0)
-        self.assertTrue(check.monitor_available(path))
+        with patch.object(check.socket, 'socket'):
+            self.assertTrue(check.monitor_available(path))
         path.lstat.return_value.st_mode |= 0o066
         self.assertFalse(check.monitor_available(path))
         path.lstat.return_value.st_mode = stat.S_IFREG | 0o600
@@ -186,6 +187,30 @@ class UpdateCheckTests(unittest.TestCase):
         path.parent.lstat.return_value.st_uid = 1000
         self.assertFalse(check.monitor_available(path))
 
+    def test_socket_left_by_a_stopped_monitor_is_not_available(self):
+        folder = Path(tempfile.mkdtemp(dir='/tmp')); self.addCleanup(lambda: __import__('shutil').rmtree(folder))
+        folder.chmod(0o700); path = folder / 'check.sock'
+        listener = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM); listener.bind(str(path)); path.chmod(0o600)
+        listener.listen(1)
+        owner = SimpleNamespace(st_uid=0)
+        real_lstat = Path.lstat
+        def lstat(self):
+            value = real_lstat(self)
+            return SimpleNamespace(st_mode=value.st_mode, st_uid=owner.st_uid)
+        with patch.object(Path, 'lstat', lstat):
+            self.assertTrue(check.monitor_available(path))
+            listener.close()  # The socket file stays, nothing listens.
+            self.assertTrue(path.exists())
+            self.assertFalse(check.monitor_available(path))
+
+    def test_failed_application_keeps_no_older_result(self):
+        self.record(); self.sidecar(update_available=True)
+        self.assertEqual([r['vmid'] for r in self.status()['available']], [110])
+        state = self.status(failed={110})
+        self.assertEqual(state['available'], [])
+        self.assertEqual(state['unknown'], 1)
+        self.assertTrue(state['checked'])
+
     def test_full_or_closed_progress_pipe_does_not_abort_client(self):
         for error in (BlockingIOError(), BrokenPipeError()):
             with patch.object(check.os, 'write', side_effect=error), patch.object(check.sys, 'stdout'):
@@ -205,7 +230,7 @@ class BridgeTests(unittest.TestCase):
         module = self.module(); events = []
         bridge.check_applications(module, [{'vmid': 110, 'installation_id': 'id-110'}], events.append)
         module.check_app.assert_called_once_with(110, 'native-app', force=True, notify=False)
-        self.assertEqual(events, [{'completed': 0, 'total': 1}, {'completed': 1, 'total': 1}, {'done': True}])
+        self.assertEqual(events, [{'completed': 0, 'total': 1}, {'completed': 1, 'total': 1}, {'done': True, 'failed': []}])
 
     def test_replaced_guest_pending_operation_and_secondary_are_not_queried(self):
         for changes, secondary in (({'installation_id': 'replacement'}, False),
@@ -236,10 +261,10 @@ class BridgeTests(unittest.TestCase):
         bridge.check_applications(module, [{'vmid': 110, 'installation_id': 'id-110'},
                                           {'vmid': 111, 'installation_id': 'id-111'}], report)
         self.assertEqual(events, [{'completed': 0, 'total': 2}, {'completed': 1, 'total': 2},
-                                  {'completed': 2, 'total': 2}, {'done': True}])
+                                  {'completed': 2, 'total': 2}, {'done': True, 'failed': []}])
         self.assertEqual(module.check_app.call_count, 2)
 
-    def test_unexpected_failure_does_not_skip_other_apps_or_report_success(self):
+    def test_unexpected_failure_does_not_skip_other_apps_and_names_the_failed_app(self):
         module = self.module(); events = []
         module._read_oci_record.side_effect = lambda vmid: {
             'vmid': vmid, 'installation_id': f'id-{vmid}', 'status': 'installed'}
@@ -248,7 +273,7 @@ class BridgeTests(unittest.TestCase):
                                           {'vmid': 111, 'installation_id': 'id-111'}], events.append)
         self.assertEqual(module.check_app.call_count, 2)
         self.assertEqual(events[-2], {'completed': 2, 'total': 2})
-        self.assertEqual(events[-1], {'error': 'check_failed'})
+        self.assertEqual(events[-1], {'done': True, 'failed': [110]})
 
     def test_requests_cannot_specify_commands_images_paths_or_duplicate_guests(self):
         valid = {'vmid': 110, 'installation_id': 'id'}

oci/tests/test_update_menu.py

@@ -134,7 +134,7 @@ class StatusTests(unittest.TestCase):
             state = checker.status()
         self.assertEqual(checker.label(state), 'Check for updates')
         self.assertEqual(state['available'], [])
-        self.assertEqual(checker.notice(state), '')
+        self.assertEqual(checker.notice(state), 'The update check could not be completed. Try again later.')
 
     def test_finished_worker_keeps_the_final_redraw_pending_until_status_is_consumed(self):
         checker = self.checker(); checker.completed = checker.total = 2
@@ -163,6 +163,29 @@ class StatusTests(unittest.TestCase):
         self.assertEqual(args.kwargs['stdout'], subprocess.PIPE)
         blocking.assert_called_once_with(checker.process.stdout.fileno(), False)
 
+    def test_failed_applications_are_reported_and_cleared_by_a_new_check(self):
+        checker = self.checker(); read, write = os.pipe()
+        os.write(write, b'{"completed": 2, "total": 2}\n{"failed": [110]}\n'); os.close(write)
+        os.set_blocking(read, False)
+        checker.process = Mock(stdout=Mock(fileno=Mock(return_value=read)))
+        checker.process.poll.return_value = checker.process.wait.return_value = 0
+        state = {'checked': True, 'available': [], 'unknown': 1, 'applications': 2}
+        with patch.object(checker.backend, 'cached', return_value=state) as cached:
+            final = checker.status()
+        os.close(read)
+        self.assertEqual(cached.call_args.kwargs['failed'], {110})
+        self.assertEqual(checker.notice(final),
+                         'Some applications could not be checked (1). Try again later.')
+        with patch.object(subprocess, 'Popen'), patch.object(updates.os, 'set_blocking'), \
+                patch.object(checker.backend, 'monitor_available', return_value=True), \
+                patch.object(checker.backend, 'cached', return_value=state):
+            checker.start()
+        self.assertEqual(checker.failed_applications, set())
+
+    def test_a_failed_check_is_reported_in_the_menu(self):
+        checker = self.checker(); checker.failed = True
+        self.assertEqual(checker.notice({}), 'The update check could not be completed. Try again later.')
+
     def test_missing_monitor_never_starts_a_fallback_checker(self):
         checker = self.checker()
         with patch.object(checker.backend, 'monitor_available', return_value=False), \