Fixes missing notification for deletion of watched dir on macOS

[samschott]

This PR fixes an issue which would prevent us from being notified when the watched directory is deleted. This is achieved by passing the kFSEventStreamCreateFlagWatchRoot to the watch.

[CCP-Aporia]

First of all thanks for this PR! However, there appear to be a few gotchas with kFSEventStreamCreateFlagWatchRoot as far as I can see:

According to Apple's file system events documentation then RootChanged indicates more than just a deletion.
Also note that it resets the event ID to 0 in case of a move, so there might be more work required in the FSEventsEmitter to not break.

I'll take a look tomorrow to whip up a few test cases to see what happens (on Big Sur at least, no access to older versions here).

[samschott]

Indeed, RootChanged will be emitted if the watched folder is moved and not deleted. A proper deletion is emitted when the watched folder is deleted (only with kFSEventStreamCreateFlagWatchRoot enabled, though). Keeping with the watchdog API, which does not allow for a watch to be dynamically moved to a new folder, I thought it best to treat RootChanged as a deleted event (like any other rename to a destination outside of watch.path).

Also, the event ID is not "reset". Only the RootChanged event itself will have an ID of zero, all other file system events will continue to have global and continuously increasing event IDs.

[BoboTiG]

Thank a lot @samschott !

I will wait for @CCP-Aporia approval. In the meantime, could you rebase your PR 🙏 ?

[CCP-Aporia]

If the public API should simply be that a changed root always translates into a DirDeleted event, then this is certainly fine and just misses a few tests (I'm currently working on some). 🙂

[samschott]

Rebased. @BoboTiG, could you confirm what the expected behaviour is for a moved root? I was merely extrapolating from the behaviour when child paths are moved out of root.

[samschott]

@CCP-Aporia, there is already a test for the root being deleted (which is failing without this PR) but no test currently for the root being renamed.

[BoboTiG]

I think we need a unified behavior.
On Windows:

watchdog/src/watchdog/observers/read_directory_changes.py

Lines 123 to 124 in 44b9618

	elif winapi_event.is_removed_self:
	self.stop()

On Linux:

watchdog/src/watchdog/observers/inotify_c.py

Lines 310 to 314 in 44b9618

	try:
	event_buffer = os.read(self._inotify_fd, event_buffer_size)
	except OSError as e:
	if e.errno == errno.EINTR:
	continue

and

watchdog/src/watchdog/observers/inotify_c.py

Line 521 in 44b9618

def is_delete_self(self):

is never used.

So on other OSes, we just skip the event and even stop the observer on Windows.

Maybe this is not ideal 🤔

Should we just fire a DirDeletedEvent and stop the observer?

[BoboTiG]

For a moved root, I would say this is the same behavior. It should just fire the DirMovedEventevent and stop the observer. But I really need inputs because this may not be the better idea.

[samschott]

I think it's definitely good to fire a DirDeletedEvent before stopping the observer when the root is deleted. Not so sure about moving, a DirMovedEvent may be more appropriate because it's more informative.

[samschott]

Always firing a DirDeletedEvent will be easier to implement because finding out where the directory moved to requires some more work, at least on macOS. From their docs here:

If you need to figure out where the directory moved, you should open the root directory with open, then pass F_GETPATH to fcntl to find its current path. See the manual page for fcntl for more information.

[CCP-Aporia]

Agreeing with @samschott on the separate events for the separate scenarios. And I'd even go as far as updating the watched path in the observer in case of a DirMovedEvent assuming that is feasible on all platforms that expose this behaviour.

[BoboTiG]

We can start light with only targeting the deleted root in that PR. Then, as the rename/move root is more work, it would be good to tackle it in a separate branch.

@samschott can you ensure the behavior will be the same on all implementation?

[samschott]

Will do. I have expanded the tests to check for a DirDeletedEvent on all platforms now and to check that the emitter thread is stopped. I have also adapted the Windows emitter to actually emit a DirDeletedEvent.

From looking at the code, it seems to me that the inotify backend already emits a DirDeletedEvent:

watchdog/src/watchdog/observers/inotify_buffer.py

Lines 93 to 98 in 44b9618

	self._queue.put(inotify_event, delay)
	if not isinstance(inotify_event, tuple) and inotify_event.is_delete_self and \
	inotify_event.src_path == self._inotify.path:
	# Deleted the watched directory, stop watching for events
	deleted_self = True

Edit: The polling backend is also already in line, though catching all OSErrors here may be a bit broad:

watchdog/src/watchdog/observers/polling.py

Lines 90 to 95 in 44b9618

	try:
	new_snapshot = self._take_snapshot()
	except OSError:
	self.queue_event(DirDeletedEvent(self.watch.path))
	self.stop()
	return

Lets see what the tests give...

[samschott]

There was a remaining issue with propagating the deleted event in inotify. Apart from this, test_delete_self should be passing on all platforms.

However, there remain issues with moving the root folder which will be difficult to solve. This is why I have removed test_move_self for now and would propose to tackle this in a separate PR. In particular:

1. On Windows, the watch is created with a handle to the directory and not the path. When deleting the underlying directory, this raises an error which is caught in winapi.py and used to generate the deleted event:
   

   watchdog/src/watchdog/observers/winapi.py

   Lines 343 to 349 in 44b9618

   	except OSError as e:
   	if e.winerror == ERROR_OPERATION_ABORTED:
   	return [], 0
   	# Handle the case when the root path is deleted
   	if _is_observed_path_deleted(handle, path):
   	return _generate_observed_path_deleted_event()

   However, the watch seemingly happily continues when the root directory is moved without emitting any event. This follows the documented behaviour here. I do not have access to a Windows machine so cannot test if this is also the case when moving the root to trash.

2. On inotify, we also don't receive a native event when the root is moved. This is problematic because it does inlcude moving the root to the trash (which is how end users typically delete a folder...). Fortunately, we can change this easily by requesting to be notified for IN_MOVE_SELF events.

3. On macOS there is the known "problem" of being notified that the root changed but not knowing if it was deleted (os.path.exists should do the trick here) or moved and where it was moved to. As previously quoted, Apple docs recommend opening the root with open and passing F_GETPATH to fcntl to find its new path. This should be easy enough apart from Python not exposing F_GETPATH (see here). It's probably best to do this in the extension module instead.

To summarise, there are clear paths forward on FSEvents and Inotify and possibly a way on Windows. However, it involves moderate amounts of work and a choice of API: Do we want to emit a DirDeletedEvent or a DirMovedEvent? In case of the latter, do we want do move the watch or cancel it? To stay consistent with the API when an item is moved outside the root path, I would probably favour just emitted a DirDeletedEvent and leaving it at that. But of course it's not my choice!

In any case, dealing with a moved root is probably the topic of a separate PR.

[samschott]

I just had a look at the test failures and none of them seem related to the changes in this PR. @BoboTiG, if you agree on postponing the handling of a moved root to a future PR (after making a decision on a uniform API), could you have a look at the changes here?

One last note on handling a moved root: In appears that the inotify backend already exposes a InotifyEvent.is_move_self attribute, it's just not used by the emitter. So it's really only the fsevents and winapi backends that need more work.

[CCP-Aporia]

Let's get this merged. :-)

[BoboTiG]

Hm those Linux tests are now always failing:

FAILED tests/test_emitter.py::test_fast_subdirectory_creation_deletion
FAILED tests/test_inotify_c.py::test_late_double_deletion
FAILED tests/test_inotify_c.py::test_watch_file

Could you have a look?

[samschott]

@BoboTiG, thanks for flagging that. There was actually a missing check that is_delete_self applied to the root path and not any of the children.

[BoboTiG]

Great job, thanks 💯