Fix FileWatcher skipping DELETE events

The FileWatcher was skipping DELETE events, except on very weird cases
where the file was deleted and re-created just before the event was
triggered by awatch().

This also extends the integration test to test both situations (delete
events when a file exist and when it doesn't).

Because there is usually a considerable delay between a change in the
filesystem happens and awatch() emits an event it is difficult to decide
if we should filter events based on the file existing or not. For
example, if a create event is received but before is triggered the file
was removed, should we emit the create event or not? Or the other way
around, should we emit a delete event if a file was created again before
the event was triggered.

It is probably best to leave this decision to the user, and let them
deal with races.

Signed-off-by: Leandro Lucarella <[email protected]>

Author: llucax

RELEASE_NOTES.md

@@ -2,7 +2,7 @@
 
 ## Summary
 
-This release adds support to pass `None` values via channels and revamps the `Timer` class to support custom policies for handling missed ticks and use the loop monotonic clock.
+This release adds support to pass `None` values via channels and revamps the `Timer` class to support custom policies for handling missed ticks and use the loop monotonic clock.  There is also a fix for the `FileWatcher` which includes a change in behavior when reporting changes for deleted files.
 
 ## Upgrading
 
@@ -31,6 +31,14 @@ This release adds support to pass `None` values via channels and revamps the `Ti
 
   **Note:** Before replacing this code blindly in all uses of `Timer.timeout()`, please consider using the periodic timer constructor `Timer.periodic()` if you need a timer that triggers reliable on a periodic fashion, as the old `Timer` (and `Timer.timeout()`) accumulates drift, which might not be what you want.
 
+* `FileWatcher` now will emit events even if the file doesn't exist anymore.
+
+  Because the underlying library has a considerable delay in triggering filesystem events, it can happen that, for example, a `CREATE` event is received but at the time of receiving the file doesn't exist anymore (because if was removed just after creation and before the event was triggered).
+
+  Before the `FileWatcher` will only emit events when the file exists, but this doesn't work for `DELETE` events (clearly). Given the nature of this mechanism can lead to races easily, it is better to leave it to the user to decide when these situations happen and just report all events.
+
+  Therefore, you should now check a file receiving an event really exist before trying to operate on it.
+
 ## New Features
 
 * `util.Timer` was replaced by a more generic implementation that allows for customizable policies to handle missed ticks.
@@ -40,3 +48,7 @@ This release adds support to pass `None` values via channels and revamps the `Ti
 ## Bug Fixes
 
 * `util.Select` / `util.Merge` / `util.MergeNamed`: Cancel pending tasks in `__del__` methods only if possible (the loop is not already closed).
+
+* `FileWatcher` will now report `DELETE` events correctly.
+
+  Due to a bug, before this release `DELETE` events were only reported if the file was re-created before the event was triggered.

src/frequenz/channels/util/_file_watcher.py

@@ -51,7 +51,11 @@ def __init__(
         self._awatch_stopped_exc: Optional[Exception] = None
         self._changes: Set[FileChange] = set()
 
-    def _filter_events(self, change: Change, path: str) -> bool:
+    def _filter_events(
+        self,
+        change: Change,
+        path: str,  # pylint: disable=unused-argument
+    ) -> bool:
         """Filter events based on the event type and path.
 
         Args:
@@ -61,10 +65,7 @@ def _filter_events(self, change: Change, path: str) -> bool:
         Returns:
             Whether the event should be notified.
         """
-        return (
-            change in [event_type.value for event_type in self.event_types]
-            and pathlib.Path(path).is_file()
-        )
+        return change in [event_type.value for event_type in self.event_types]
 
     def __del__(self) -> None:
         """Cleanup registered watches.

tests/utils/test_integration.py

@@ -46,9 +46,12 @@ async def test_file_watcher(tmp_path: pathlib.Path) -> None:
 
 
 @pytest.mark.integration
-async def test_file_watcher_change_types(tmp_path: pathlib.Path) -> None:
+async def test_file_watcher_deletes(tmp_path: pathlib.Path) -> None:
     """Ensure file watcher is returning paths only on the DELETE change.
 
+    Also ensures that DELETE events are sent even if the file was recreated and even if
+    the file doesn't exist.
+
     Args:
         tmp_path (pathlib.Path): A tmp directory to run the file watcher on.
             Created by pytest.
@@ -63,18 +66,44 @@ async def test_file_watcher_change_types(tmp_path: pathlib.Path) -> None:
         deletion_timer=Timer.timeout(timedelta(seconds=0.25)),
         watcher=file_watcher,
     )
-    number_of_deletes = 0
     number_of_write = 0
+    number_of_deletes = 0
+    number_of_events = 0
+    # We want to write to a file and then removed, and then write again (create it
+    # again) and remove it again and then stop.
+    # Because awatch takes some time to get notified by the OS, we need to stop writing
+    # while a delete was done, to make sure the file is not recreated before the
+    # deletion event arrives.
+    # For the second round of writes and then delete, we allow writing after the delete
+    # was done as an extra test.
+    #
+    # This is an example timeline for this test:
+    #
+    # |-----|--.--|-----|---o-|-----|--.--|-----|--o--|-----|-----|-----|-----|-----|
+    # W     W  D            E W     W  D  W     W  E
+    #
+    # Where:
+    # W: Write
+    # D: Delete
+    # E: FileWatcher Event
     while await select.ready():
         if msg := select.write_timer:
+            if number_of_write >= 2 and number_of_events == 0:
+                continue
             filename.write_text(f"{msg.inner}")
             number_of_write += 1
         elif _ := select.deletion_timer:
+            # Avoid removing the file twice
+            if not pathlib.Path(filename).is_file():
+                continue
             os.remove(filename)
-        elif _ := select.watcher:
             number_of_deletes += 1
-            break
+        elif _ := select.watcher:
+            number_of_events += 1
+            if number_of_events >= 2:
+                break
 
-    assert number_of_deletes == 1
+    assert number_of_deletes == 2
     # Can be more because the watcher could take some time to trigger
-    assert number_of_write >= 2
+    assert number_of_write >= 3
+    assert number_of_events == 2