Fix ConfigManagingActor raising unhandled exceptions when file does…

…n't exist (#1116)

This was detected when I edited config file with `vim` and
`config.toml.swp` file was created and deleted.

RELEASE_NOTES.md

@@ -6,7 +6,8 @@
 
 ## Upgrading
 
-<!-- Here goes notes on how to upgrade from previous versions, including deprecations and what they should be replaced with -->
+- The `ConfigManagingActor` now only reacts to `CREATE` and `MODIFY` events. `DELETE` is not supported anymore and are ignored.
+- Remove the `event_types` argument from the `ConfigManagingActor` constructor.
 
 ## New Features
 
@@ -17,3 +18,8 @@
 ## Bug Fixes
 
 - Fix a bug in the resampler that could end up with an *IndexError: list index out of range* exception when a new resampler was added while awaiting the existing resampler to finish resampling.
+
+- Fix bugs with `ConfigManagingActor`:
+  - Raising unhandled exceptions when any file in config directory was deleted.
+  - Raising unhandled exception if not all config files exist.
+  - Eliminate recursive actor crashes when all config files were missing.

src/frequenz/sdk/config/_config_managing.py

@@ -74,7 +74,6 @@ def __init__(
         self,
         config_paths: abc.Sequence[pathlib.Path | str],
         output: Sender[abc.Mapping[str, Any]],
-        event_types: abc.Set[EventType] = frozenset(EventType),
         *,
         name: str | None = None,
         force_polling: bool = True,
@@ -89,7 +88,6 @@ def __init__(
                 the previous paths. Dict keys will be merged recursively, but other
                 objects (like lists) will be replaced by the value in the last path.
             output: The sender to send the configuration to.
-            event_types: The set of event types to monitor.
             name: The name of the actor. If `None`, `str(id(self))` will
                 be used. This is used mostly for debugging purposes.
             force_polling: Whether to force file polling to check for changes.
@@ -106,18 +104,14 @@ def __init__(
             for config_path in config_paths
         ]
         self._output: Sender[abc.Mapping[str, Any]] = output
-        self._event_types: abc.Set[EventType] = event_types
         self._force_polling: bool = force_polling
         self._polling_interval: timedelta = polling_interval
 
-    def _read_config(self) -> abc.Mapping[str, Any]:
+    def _read_config(self) -> abc.Mapping[str, Any] | None:
         """Read the contents of the configuration file.
 
         Returns:
             A dictionary containing configuration variables.
-
-        Raises:
-            ValueError: If config file cannot be read.
         """
         error_count = 0
         config: dict[str, Any] = {}
@@ -130,16 +124,29 @@ def _read_config(self) -> abc.Mapping[str, Any]:
             except ValueError as err:
                 _logger.error("%s: Can't read config file, err: %s", self, err)
                 error_count += 1
+            except OSError as err:
+                # It is ok for config file to don't exist.
+                _logger.error(
+                    "%s: Error reading config file %s (%s). Ignoring it.",
+                    self,
+                    err,
+                    config_path,
+                )
+                error_count += 1
 
         if error_count == len(self._config_paths):
-            raise ValueError(f"{self}: Can't read any of the config files")
+            _logger.error(
+                "%s: Can't read any of the config files, ignoring config update.", self
+            )
+            return None
 
         return config
 
     async def send_config(self) -> None:
         """Send the configuration to the output sender."""
         config = self._read_config()
-        await self._output.send(config)
+        if config is not None:
+            await self._output.send(config)
 
     async def _run(self) -> None:
         """Monitor for and send configuration file updates.
@@ -157,17 +164,32 @@ async def _run(self) -> None:
         # or it is deleted and recreated again.
         file_watcher = FileWatcher(
             paths=list(parent_paths),
-            event_types=self._event_types,
+            event_types={EventType.CREATE, EventType.MODIFY},
             force_polling=self._force_polling,
             polling_interval=self._polling_interval,
         )
 
         try:
             async for event in file_watcher:
+                if not event.path.exists():
+                    _logger.error(
+                        "%s: Received event %s, but the watched path %s doesn't exist.",
+                        self,
+                        event,
+                        event.path,
+                    )
+                    continue
                 # Since we are watching the whole parent directories, we need to make
                 # sure we only react to events related to the configuration files we
                 # are interested in.
-                if not any(event.path.samefile(p) for p in self._config_paths):
+                #
+                # pathlib.Path.samefile raises error if any path doesn't exist so we need to
+                # make sure the paths exists before calling it. This could happen as it is not
+                # required that all config files exist, only one is required but we don't know
+                # which.
+                if not any(
+                    event.path.samefile(p) for p in self._config_paths if p.exists()
+                ):
                     continue
 
                 match event.type:
@@ -186,8 +208,9 @@ async def _run(self) -> None:
                         )
                         await self.send_config()
                     case EventType.DELETE:
-                        _logger.info(
-                            "%s: The configuration file %s was deleted, ignoring...",
+                        _logger.error(
+                            "%s: Unexpected DELETE event for path %s. Please report this "
+                            "issue to Frequenz.",
                             self,
                             event.path,
                         )

tests/config/test_config_manager.py

@@ -8,9 +8,12 @@
 from collections.abc import Mapping, MutableMapping
 from dataclasses import dataclass
 from typing import Any
+from unittest.mock import MagicMock
 
 import pytest
 from frequenz.channels import Broadcast
+from frequenz.channels.file_watcher import Event, EventType
+from pytest_mock import MockerFixture
 
 from frequenz.sdk.config import ConfigManagingActor
 from frequenz.sdk.config._config_managing import _recursive_update
@@ -265,6 +268,96 @@ async def test_update_multiple_files(self, config_file: pathlib.Path) -> None:
                 "dict_str_int": {"a": 1, "b": 2, "c": 4},
             }
 
+    async def test_actor_works_if_not_all_config_files_exist(
+        self, config_file: pathlib.Path
+    ) -> None:
+        """Test ConfigManagingActor works if not all config files exist."""
+        config_channel: Broadcast[Mapping[str, Any]] = Broadcast(
+            name="Config Channel", resend_latest=True
+        )
+        config_receiver = config_channel.new_receiver()
+
+        # This file does not exist
+        config_file2 = config_file.parent / "config2.toml"
+
+        async with ConfigManagingActor(
+            [config_file, config_file2],
+            config_channel.new_sender(),
+            force_polling=False,
+        ):
+            config = await config_receiver.receive()
+            assert config is not None
+            assert config.get("var2") is None
+
+            number = 5
+            config_file.write_text(create_content(number=number))
+
+            config = await config_receiver.receive()
+            assert config is not None
+            assert config.get("var2") == str(number)
+
+            # Create second config file that overrides the value from the first one
+            number = 42
+            config_file2.write_text(create_content(number=number))
+
+            config = await config_receiver.receive()
+            assert config is not None
+            assert config.get("var2") == str(number)
+
+    async def test_actor_does_not_crash_if_file_is_deleted(
+        self, config_file: pathlib.Path, mocker: MockerFixture
+    ) -> None:
+        """Test ConfigManagingActor does not crash if a file is deleted."""
+        config_channel: Broadcast[Mapping[str, Any]] = Broadcast(
+            name="Config Channel", resend_latest=True
+        )
+        config_receiver = config_channel.new_receiver()
+
+        number = 5
+        config_file2 = config_file.parent / "config2.toml"
+        config_file2.write_text(create_content(number=number))
+
+        # Not config file but existing in the same directory
+        any_file = config_file.parent / "any_file.txt"
+        any_file.write_text("content")
+
+        file_watcher_mock = MagicMock()
+        file_watcher_mock.__anext__.side_effect = [
+            Event(EventType.DELETE, any_file),
+            Event(EventType.MODIFY, config_file2),
+            Event(EventType.MODIFY, config_file),
+        ]
+
+        mocker.patch(
+            "frequenz.channels.file_watcher.FileWatcher", return_value=file_watcher_mock
+        )
+
+        async with ConfigManagingActor(
+            [config_file, config_file2],
+            config_channel.new_sender(),
+            force_polling=False,
+        ) as actor:
+            send_config_spy = mocker.spy(actor, "send_config")
+
+            config = await config_receiver.receive()
+            assert config is not None
+            assert config.get("var2") == str(number)
+            send_config_spy.assert_called_once()
+            send_config_spy.reset_mock()
+
+            # Remove file and send DELETE events
+            any_file.unlink()
+            config_file2.unlink()
+            number = 101
+            config_file.write_text(create_content(number=number))
+
+            config = await config_receiver.receive()
+            assert config is not None
+            assert config.get("var2") == str(number)
+            # Config should be updated only once on MODIFY event
+            # DELETE events are ignored
+            send_config_spy.assert_called_once()
+
 
 @dataclass(frozen=True, kw_only=True)
 class RecursiveUpdateTestCase: