fs: report errors from recursive watch events

lib/internal/fs/recursive_watch.js

@@ -157,8 +157,11 @@ class FSWatcher extends EventEmitter {
   }
 
   // Report an entry that vanished between being listed and being watched so
-  // the caller can discard its stale bookkeeping.
-  #watch(file, onChange) {
+  // the caller can discard its stale bookkeeping. Other failures propagate
+  // out of fs.watch() during the initial scan. Later, nothing could catch
+  // them, so they are reported through 'error' and the entry is discarded as
+  // well; a later event about it picks it up again.
+  #watch(file, onChange, initial) {
     if (this.#closed) {
       return false;
     }
@@ -174,7 +177,11 @@ class FSWatcher extends EventEmitter {
           (file !== this.#rootPath || !this.#options.throwIfNoEntry)) {
         return false;
       }
-      throw err;
+      if (initial) {
+        throw err;
+      }
+      this.emit('error', err);
+      return false;
     }
     this.#watchers.set(file, watcher);
     return true;
@@ -184,7 +191,7 @@ class FSWatcher extends EventEmitter {
   // yet (emitting 'rename' for them unless this is the initial scan);
   // #addEntry() descends into subdirectories.
   #scanFolder(folder, initial) {
-    if (!this.#watch(folder, (eventType, filename) => this.#onFolderEvent(folder, filename))) {
+    if (!this.#watch(folder, (eventType, filename) => this.#onFolderEvent(folder, filename), initial)) {
       this.#forget(folder);
       return;
     }
@@ -230,26 +237,36 @@ class FSWatcher extends EventEmitter {
       // The link target is watched so that changes behind the link surface
       // as a 'rename' of the link, as they always have on this code path.
       this.#symbolicLinks.add(file);
-      if (!this.#watch(file, () => this.#emit('rename', file))) {
+      if (!this.#watch(file, () => this.#emit('rename', file), initial)) {
         this.#forget(file);
       }
     } else if (entry.isDirectory()) {
       this.#scanFolder(file, initial);
     } else if (!kDirectoryWatchReportsEntries) {
-      if (!this.#watch(file, () => this.#onEntryEvent(file))) {
+      if (!this.#watch(file, () => this.#onEntryEvent(file), initial)) {
         this.#forget(file);
       }
     }
   }
 
+  // This and #onEntryEvent() run from fs.watch() callbacks, where nothing can
+  // catch an exception, so failures other than an entry being gone are
+  // reported through 'error', as #scanFolder() does for readdir().
   #onFolderEvent(folder, filename) {
     if (this.#closed) {
       return;
     }
     const { lstatSync, statSync } = lazyLoadFsSync();
     if (!kDirectoryWatchReportsEntries || filename == null) {
       // All that is known is that something about `folder` changed.
-      if (statSync(folder, { throwIfNoEntry: false }) === undefined) {
+      let stats;
+      try {
+        stats = statSync(folder, { throwIfNoEntry: false });
+      } catch (error) {
+        this.emit('error', error);
+        return;
+      }
+      if (stats === undefined) {
         this.#emit('rename', folder);
         this.#forget(folder);
       } else {
@@ -265,12 +282,31 @@ class FSWatcher extends EventEmitter {
       if (this.#ignoreMatcher?.(pathRelative(this.#rootPath, file))) {
         return;
       }
-      const entry = lstatSync(file, { throwIfNoEntry: false });
+      let entry;
+      try {
+        entry = lstatSync(file, { throwIfNoEntry: false });
+      } catch (error) {
+        // Like statSync(), take ENOTDIR (`folder` has been replaced by a
+        // non-directory) to mean that the entry is gone.
+        if (error.code !== 'ENOTDIR') {
+          this.emit('error', error);
+          return;
+        }
+      }
       if (entry !== undefined) {
         this.#addEntry(file, entry, false);
-      } else if (folder === this.#rootPath && statSync(folder, { throwIfNoEntry: false }) === undefined) {
-        this.#emit('rename', folder);
-        this.#forget(folder);
+      } else if (folder === this.#rootPath) {
+        let stats;
+        try {
+          stats = statSync(folder, { throwIfNoEntry: false });
+        } catch (error) {
+          this.emit('error', error);
+          return;
+        }
+        if (stats === undefined) {
+          this.#emit('rename', folder);
+          this.#forget(folder);
+        }
       }
       return;
     }
@@ -284,7 +320,13 @@ class FSWatcher extends EventEmitter {
       return;
     }
     const { statSync } = lazyLoadFsSync();
-    const stats = statSync(file, { throwIfNoEntry: false });
+    let stats;
+    try {
+      stats = statSync(file, { throwIfNoEntry: false });
+    } catch (error) {
+      this.emit('error', error);
+      return;
+    }
     if (stats === undefined) {
       this.#emit('rename', file);
       this.#forget(file);
@@ -310,7 +352,7 @@ class FSWatcher extends EventEmitter {
       } else {
         this.emit('change', 'change', pathBasename(file));
       }
-    })) {
+    }, true)) {
       this.#forget(file);
     }
   }

test/parallel/test-fs-watch-recursive-dir-replaced-by-file.js

@@ -0,0 +1,35 @@
+'use strict';
+
+const common = require('../common');
+
+// Events naming entries of a watched directory that has meanwhile been
+// replaced by a file must not make the recursive watcher throw from inside
+// its own fs.watch() callbacks: those entries are simply gone.
+
+if (!common.isLinux)
+  common.skip('this test exercises the inotify-based recursive watcher');
+
+const fs = require('fs');
+const path = require('path');
+const tmpdir = require('../common/tmpdir');
+
+tmpdir.refresh();
+
+const root = tmpdir.resolve('root');
+const sub = path.join(root, 'sub');
+fs.mkdirSync(sub, { recursive: true });
+
+const watcher = fs.watch(root, { recursive: true });
+watcher.on('error', common.mustNotCall());
+watcher.on('change', common.mustCallAtLeast((eventType, filename) => {
+  // Inotify reports events in order, so by the time the file created last is
+  // reported, the events naming entries of `sub` have been handled.
+  if (filename === 'after') {
+    watcher.close();
+  }
+}));
+
+fs.writeFileSync(path.join(sub, 'file'), '');
+fs.rmSync(sub, { recursive: true });
+fs.writeFileSync(sub, '');
+fs.writeFileSync(path.join(root, 'after'), '');

test/parallel/test-fs-watch-recursive-symlink-loop.js

@@ -0,0 +1,37 @@
+'use strict';
+
+const common = require('../common');
+
+// A symbolic link that cannot be watched (here, one of a pair pointing at
+// each other) appearing in a watched tree must be reported through 'error'
+// instead of being thrown from inside the recursive watcher's own fs.watch()
+// callbacks, and the watcher must keep working.
+
+if (!common.isLinux)
+  common.skip('this test exercises the inotify-based recursive watcher');
+
+const assert = require('assert');
+const fs = require('fs');
+const path = require('path');
+const tmpdir = require('../common/tmpdir');
+
+tmpdir.refresh();
+
+const root = tmpdir.resolve('root');
+fs.mkdirSync(root);
+
+const watcher = fs.watch(root, { recursive: true });
+watcher.on('error', common.mustCallAtLeast((error) => {
+  assert.strictEqual(error.code, 'ELOOP');
+}));
+watcher.on('change', common.mustCallAtLeast((eventType, filename) => {
+  // Inotify reports events in order, so by the time the file created last is
+  // reported, the events about the links have been handled.
+  if (filename === 'after') {
+    watcher.close();
+  }
+}));
+
+fs.symlinkSync(path.join(root, 'b'), path.join(root, 'a'));
+fs.symlinkSync(path.join(root, 'a'), path.join(root, 'b'));
+fs.writeFileSync(path.join(root, 'after'), '');

test/parallel/test-fs-watch-recursive-unreadable-dir.js

@@ -0,0 +1,82 @@
+'use strict';
+
+const common = require('../common');
+
+// The recursive watcher must report directories it cannot look into through
+// 'error' instead of throwing from inside its own fs.watch() callbacks, where
+// nothing can catch the exception, and must keep working afterwards.
+
+if (!common.isLinux)
+  common.skip('this test exercises the inotify-based recursive watcher');
+
+if (process.getuid() === 0)
+  common.skip('as this test should not be run as `root`');
+
+const assert = require('assert');
+const fs = require('fs');
+const path = require('path');
+const tmpdir = require('../common/tmpdir');
+
+const root = tmpdir.resolve('root');
+const existing = path.join(root, 'existing');
+const created = path.join(root, 'created');
+
+// Registered before tmpdir.refresh() so that it runs before tmpdir's own
+// 'exit' cleanup, which cannot remove a directory it is unable to read.
+process.on('exit', () => {
+  for (const dir of [existing, created]) {
+    try {
+      fs.chmodSync(dir, 0o755);
+    } catch {
+      // Never created.
+    }
+  }
+});
+
+tmpdir.refresh();
+fs.mkdirSync(existing, { recursive: true });
+
+// Resolves once `filename` is reported. Inotify reports events in order, so
+// by then the events caused before it have been handled.
+function reported(watcher, filename) {
+  return new Promise((resolve) => {
+    watcher.on('change', function onChange(eventType, name) {
+      if (name === filename) {
+        watcher.off('change', onChange);
+        resolve();
+      }
+    });
+  });
+}
+
+(async () => {
+  {
+    // A watched directory that becomes unreadable.
+    const watcher = fs.watch(root, { recursive: true });
+    watcher.on('error', common.mustCallAtLeast((error) => {
+      assert.strictEqual(error.code, 'EACCES');
+    }));
+    const after = reported(watcher, 'after');
+    fs.chmodSync(existing, 0o000);
+    fs.writeFileSync(path.join(root, 'after'), '');
+    await after;
+    watcher.close();
+    fs.chmodSync(existing, 0o755);
+  }
+
+  {
+    // A directory that is already unreadable when its creation is handled is
+    // picked up once it can be read.
+    const watcher = fs.watch(root, { recursive: true });
+    watcher.on('error', common.mustCallAtLeast((error) => {
+      assert.strictEqual(error.code, 'EACCES');
+      fs.chmodSync(created, 0o755);
+    }));
+    const inside = reported(watcher, path.join('created', 'inside'));
+    fs.mkdirSync(created);
+    fs.writeFileSync(path.join(created, 'inside'), '');
+    fs.chmodSync(created, 0o000);
+    await inside;
+    watcher.close();
+  }
+})().then(common.mustCall());