Fix Linux watcher for entities recently moved between directories (#2248)

Author: davidmorgan

pkgs/watcher/CHANGELOG.md

@@ -10,15 +10,16 @@
   exhaustion, "Directory watcher closed unexpectedly", much less likely. The old
   implementation which does not use a separate Isolate is available as
   `DirectoryWatcher(path, runInIsolateOnWindows: false)`.
-- Bug fix: fix tracking failure on Linux. Before the fix, renaming a directory
-  would cause subdirectories of that directory to no longer be tracked.
-- Bug fix: while listing directories skip symlinks that lead to a directory
-  that has already been listed. This prevents a severe performance regression on
-  MacOS and Linux when there are more than a few symlink loops.
-- Bug fix: with `FileWatcher` on MacOS, a modify event was sometimes reported if
-  the file was created immediately before the watcher was created. Now, if the
-  file exists when the watcher is created then this modify event is not sent.
-  This matches the Linux native and polling (Windows) watchers.
+- Bug fix: fix `DirectoryWatcher` tracking failure on Linux. Before the fix,
+  renaming a directory would cause subdirectories of that directory to no
+  longer be tracked.
+- Bug fix: fix `DirectoryWatcher` incorrect events on Linux when a file or
+  directory is moved between directories then immediately modified or deleted.
+- Bug fix: fix `DirectoryWatcher` duplicate ADD events on Linux when a file
+  is created in a recently moved or created directory.
+- Bug fix: in `DirectoryWatcher` while listing directories skip symlinks that
+  lead to a directory that has already been listed. This prevents a severe
+  performance regression on MacOS and Linux when there are more than a few symlink loops.
 - Bug fix: with `DirectoryWatcher` on Windows, the last of a rapid sequence of
   modifications in a newly-created directory was sometimes dropped. Make it
   reliably report the last modification.
@@ -30,8 +31,14 @@
 - Bug fix: with `DirectoryWatcher` on Windows, new links to direcories were
   sometimes incorrectly handled as actual directories. Now they are reported
   as files, matching the behavior of the Linux and MacOS watchers.
+- Bug fix: with `DirectoryWatcher` on MacOS, fix events for changes in new
+  directories: don't emit duplicate ADD, don't emit MODIFY without ADD.
 - Bug fix: with `PollingDirectoryWatcher`, fix spurious modify event emitted
   because of a file delete during polling.
+- Bug fix: with `FileWatcher` on MacOS, a modify event was sometimes reported if
+  the file was created immediately before the watcher was created. Now, if the
+  file exists when the watcher is created then this modify event is not sent.
+  This matches the Linux native and polling (Windows) watchers.
 - Document behavior on Linux if the system watcher limit is hit.
 
 ## 1.1.4

pkgs/watcher/lib/src/directory_watcher/directory_list.dart

@@ -142,16 +142,12 @@ class _ResolvedDirectory {
     } on FileSystemException catch (e, s) {
       // The first operation on a directory is to resolve symbolic links, which
       // fails with a general FileSystemException if the file is not found.
-      // Convert that into a PathNotFoundException or PathAccessException
-      // as that makes more sense to the caller, who didn't ask for anything to
-      // do with symbolic links.
+      // See https://github.com/dart-lang/sdk/issues/61946. Use a copy of the
+      // SDK codepath that should convert it to a more specific type.
       if (e.message.contains('Cannot resolve symbolic links')) {
-        if (e.osError?.errorCode == 2) {
-          throw Error.throwWithStackTrace(
-              PathNotFoundException(directory.path, e.osError!), s);
-        } else if (e.osError?.errorCode == 5) {
-          throw Error.throwWithStackTrace(
-              PathAccessException(directory.path, e.osError!), s);
+        if (e.osError != null && e.path != null) {
+          Error.throwWithStackTrace(
+              _fromOSError(e.osError!, e.message, e.path!), s);
         }
       }
       rethrow;
@@ -160,3 +156,74 @@ class _ResolvedDirectory {
 
   bool get isCanonical => canonicalPath == directory.path;
 }
+
+// Copied from sdk/lib/io/file.dart.
+FileSystemException _fromOSError(OSError err, String message, String path) {
+  if (Platform.isWindows) {
+    switch (err.errorCode) {
+      case _errorAccessDenied:
+      case _errorCurrentDirectory:
+      case _errorWriteProtect:
+      case _errorBadLength:
+      case _errorSharingViolation:
+      case _errorLockViolation:
+      case _errorNetworkAccessDenied:
+      case _errorDriveLocked:
+        return PathAccessException(path, err, message);
+      case _errorFileExists:
+      case _errorAlreadyExists:
+        return PathExistsException(path, err, message);
+      case _errorFileNotFound:
+      case _errorPathNotFound:
+      case _errorInvalidDrive:
+      case _errorInvalidName:
+      case _errorNoMoreFiles:
+      case _errorBadNetpath:
+      case _errorBadNetName:
+      case _errorBadPathName:
+      case _errorFilenameExedRange:
+        return PathNotFoundException(path, err, message);
+      default:
+        return FileSystemException(message, path, err);
+    }
+  } else {
+    switch (err.errorCode) {
+      case _ePerm:
+      case _eAccess:
+        return PathAccessException(path, err, message);
+      case _eExist:
+        return PathExistsException(path, err, message);
+      case _eNoEnt:
+        return PathNotFoundException(path, err, message);
+      default:
+        return FileSystemException(message, path, err);
+    }
+  }
+}
+
+// Copied from sdk/lib/io/common.dart. POSIX.
+const _ePerm = 1;
+const _eNoEnt = 2;
+const _eAccess = 13;
+const _eExist = 17;
+
+// Copied from sdk/lib/io/common.dart. Windows.
+const _errorFileNotFound = 2;
+const _errorPathNotFound = 3;
+const _errorAccessDenied = 5;
+const _errorInvalidDrive = 15;
+const _errorCurrentDirectory = 16;
+const _errorNoMoreFiles = 18;
+const _errorWriteProtect = 19;
+const _errorBadLength = 24;
+const _errorSharingViolation = 32;
+const _errorLockViolation = 33;
+const _errorBadNetpath = 53;
+const _errorNetworkAccessDenied = 65;
+const _errorBadNetName = 67;
+const _errorFileExists = 80;
+const _errorDriveLocked = 108;
+const _errorInvalidName = 123;
+const _errorBadPathName = 161;
+const _errorAlreadyExists = 183;
+const _errorFilenameExedRange = 206;

pkgs/watcher/lib/src/directory_watcher/linux.dart

@@ -154,10 +154,14 @@ class _LinuxDirectoryWatcher
     var dirs = <String>{};
     var changed = <String>{};
 
-    // inotify event batches are ordered by occurrence, so we treat them as a
-    // log of what happened to a file. We only emit events based on the
-    // difference between the state before the batch and the state after it, not
-    // the intermediate state.
+    // Inotify events are usually ordered by occurrence. But,
+    // https://github.com/dart-lang/sdk/issues/62014 means that moves between
+    // directories cause create/delete events to be placed out of order at the
+    // end of the batch. Catch these cases in order to do a check on the actual
+    // filesystem state.
+    var deletes = <String>{};
+    var creates = <String>{};
+
     for (var event in batch) {
       // If the watched directory is deleted or moved, we'll get a deletion
       // event for it. Ignore it; we handle closing [this] when the underlying
@@ -168,41 +172,71 @@ class _LinuxDirectoryWatcher
 
       switch (event.type) {
         case EventType.moveFile:
+          deletes.add(event.path);
           files.remove(event.path);
           dirs.remove(event.path);
           var destination = event.destination;
           if (destination != null) {
+            creates.add(destination);
             changed.add(destination);
             files.add(destination);
             dirs.remove(destination);
           }
 
         case EventType.moveDirectory:
+          deletes.add(event.path);
           files.remove(event.path);
           dirs.remove(event.path);
           var destination = event.destination;
           if (destination != null) {
+            creates.add(destination);
             changed.add(destination);
             files.remove(destination);
             dirs.add(destination);
           }
 
         case EventType.delete:
+          deletes.add(event.path);
           files.remove(event.path);
           dirs.remove(event.path);
 
         case EventType.createDirectory:
+          creates.add(event.path);
+          files.remove(event.path);
+          dirs.add(event.path);
+
         case EventType.modifyDirectory:
           files.remove(event.path);
           dirs.add(event.path);
 
         case EventType.createFile:
+          creates.add(event.path);
+          files.add(event.path);
+          dirs.remove(event.path);
+
         case EventType.modifyFile:
           files.add(event.path);
           dirs.remove(event.path);
       }
     }
 
+    // Check paths that might have been affected by out-of-order events, set
+    // the correct state in [files] and [dirs].
+    for (final path in deletes.intersection(creates)) {
+      final type = FileSystemEntity.typeSync(path, followLinks: false);
+      if (type == FileSystemEntityType.file ||
+          type == FileSystemEntityType.link) {
+        files.add(path);
+        dirs.remove(path);
+      } else if (type == FileSystemEntityType.directory) {
+        dirs.add(path);
+        files.remove(path);
+      } else {
+        files.remove(path);
+        dirs.remove(path);
+      }
+    }
+
     _applyChanges(files, dirs, changed);
   }
 
@@ -244,8 +278,12 @@ class _LinuxDirectoryWatcher
       if (entity is Directory) {
         _watchSubdir(entity.path);
       } else {
-        _files.add(entity.path);
-        _emitEvent(ChangeType.ADD, entity.path);
+        // Only emit ADD if it hasn't already been emitted due to the file being
+        // modified or added after the directory was added.
+        if (!_files.contains(entity.path)) {
+          _files.add(entity.path);
+          _emitEvent(ChangeType.ADD, entity.path);
+        }
       }
     }, onError: (Object error, StackTrace stackTrace) {
       // Ignore an exception caused by the dir not existing. It's fine if it

pkgs/watcher/lib/src/directory_watcher/mac_os.dart

@@ -136,10 +136,10 @@ class _MacOSDirectoryWatcher
       for (var event in events) {
         switch (event.type) {
           case EventType.createFile:
-            // If we already know about the file, treat it like a modification.
-            // This can happen if a file is copied on top of an existing one.
-            // We'll see an ADD event for the latter file when from the user's
-            // perspective, the file's contents just changed.
+          case EventType.modifyFile:
+            // The type can be incorrect due to a race with listing a new
+            // directory or due to a file being copied over an existing one.
+            // Choose the type to emit based on the previous emitted state.
             var type =
                 _files.contains(path) ? ChangeType.MODIFY : ChangeType.ADD;
 
@@ -152,7 +152,7 @@ class _MacOSDirectoryWatcher
             var stream = Directory(path).listRecursivelyIgnoringErrors();
             var subscription = stream.listen((entity) {
               if (entity is Directory) return;
-              if (_files.contains(path)) return;
+              if (_files.contains(entity.path)) return;
 
               _emitEvent(ChangeType.ADD, entity.path);
               _files.add(entity.path);
@@ -163,9 +163,6 @@ class _MacOSDirectoryWatcher
             subscription.onError(_emitError);
             _listSubscriptions.add(subscription);
 
-          case EventType.modifyFile:
-            _emitEvent(ChangeType.MODIFY, path);
-
           case EventType.delete:
             for (var removedPath in _files.remove(path)) {
               _emitEvent(ChangeType.REMOVE, removedPath);

pkgs/watcher/test/directory_watcher/end_to_end_tests.dart

@@ -27,20 +27,20 @@ void endToEndTests({required bool isNative}) {
     final temp = Directory.systemTemp.createTempSync();
     addTearDown(() => temp.deleteSync(recursive: true));
 
-    // Start with some files.
-    final changer = FileChanger(temp.path);
-    await changer.changeFiles(times: 100);
-
     // Create the watcher and [ClientSimulator].
     final watcher = createWatcher(path: temp.path);
     final client = await ClientSimulator.watch(watcher);
     addTearDown(client.close);
 
     // 20 iterations of making changes, waiting for events to settle, and
     // checking for consistency.
-    for (var i = 0; i != 20; ++i) {
+    final changer = FileChanger(temp.path);
+    for (var i = 0; i != 40; ++i) {
+      for (final entity in temp.listSync()) {
+        entity.deleteSync(recursive: true);
+      }
       // File changes.
-      final messages = await changer.changeFiles(times: 100);
+      final messages = await changer.changeFiles(times: 200);
 
       // Give time for events to arrive. To allow tests to run quickly when the
       // events are handled quickly, poll and continue if verification passes.

pkgs/watcher/test/directory_watcher/file_changer.dart

@@ -8,6 +8,8 @@ import 'dart:math';
 
 import 'package:path/path.dart' as p;
 
+import '../utils.dart';
+
 /// Changes files randomly.
 ///
 /// Writes are done in an isolate so as to not block the watcher code being
@@ -23,16 +25,21 @@ import 'package:path/path.dart' as p;
 class FileChanger {
   final String path;
 
-  final Random _random = Random(0);
+  Random _random = Random(0);
   final List<String> _messages = [];
 
   FileChanger(this.path);
 
   /// Changes files under [path], [times] times.
   ///
   /// Returns a log of the changes made.
-  Future<List<String>> changeFiles({required int times}) async =>
-      await Isolate.run(() => _changeFiles(times: times));
+  Future<List<String>> changeFiles({required int times}) async {
+    final result = await Isolate.run(() => _changeFiles(times: times));
+    // The `Random` instance gets copied to the isolate on every run, so by
+    // default it will produce the same numbers. Update it to get new numbers.
+    _random = Random(_random.nextInt(0xffffffff));
+    return result;
+  }
 
   Future<List<String>> _changeFiles({required int times}) async {
     _messages.clear();
@@ -82,7 +89,7 @@ class FileChanger {
         final existingPath2 = _randomExistingFilePath()!;
         _log('move file over file,$existingPath,$existingPath2');
         // Fails sometimes on Windows, so guard+retry.
-        _retryForPathAccessException(
+        retryForPathAccessException(
             () => File(existingPath).renameSync(existingPath2));
 
       case 6:
@@ -94,7 +101,7 @@ class FileChanger {
         _ensureParent(newDirectory);
         _log('move directory to new,$existingDirectory,$newDirectory');
         // Fails sometimes on Windows, so guard+retry.
-        _retryForPathAccessException(
+        retryForPathAccessException(
             () => Directory(existingDirectory).renameSync(newDirectory));
 
       case 7:
@@ -133,6 +140,7 @@ class FileChanger {
   /// Returns the path to an already-created file, or `null` if none exists.
   String? _randomExistingFilePath() =>
       (Directory(path).listSync(recursive: true).whereType<File>().toList()
+            ..sort((a, b) => a.path.compareTo(b.path))
             ..shuffle(_random))
           .firstOrNull
           ?.path;
@@ -142,6 +150,7 @@ class FileChanger {
   String? _randomExistingDirectoryPath() => (Directory(
         path,
       ).listSync(recursive: true).whereType<Directory>().toList()
+            ..sort((a, b) => a.path.compareTo(b.path))
             ..shuffle(_random))
           .firstOrNull
           ?.path;
@@ -156,16 +165,4 @@ class FileChanger {
     message = message.replaceAll(',$path${Platform.pathSeparator}', ',');
     _messages.add(message);
   }
-
-  /// Retries [action] until it does not throw [PathAccessException].
-  void _retryForPathAccessException(void Function() action) {
-    while (true) {
-      try {
-        action();
-        return;
-      } on PathAccessException catch (e) {
-        print('Temporary failure, retrying: $e');
-      }
-    }
-  }
 }