test: make fs.watch errors more strict

lib/internal/fs/recursive_watch.js

@@ -6,9 +6,8 @@ const {
   Promise,
   PromisePrototypeThen,
   SafeMap,
+  SafeSet,
   StringPrototypeStartsWith,
-  StringPrototypeSlice,
-  StringPrototypeLastIndexOf,
   SymbolAsyncIterator,
 } = primordials;
 
@@ -39,7 +38,7 @@ function lazyLoadFsSync() {
   return internalSync;
 }
 
-async function traverse(dir, files = new SafeMap()) {
+async function traverse(dir, files = new SafeMap(), symbolicLinks = new SafeSet()) {
   const { opendir } = lazyLoadFsPromises();
 
   const filenames = await opendir(dir);
@@ -50,8 +49,9 @@ async function traverse(dir, files = new SafeMap()) {
 
     files.set(f, file);
 
+    // Do not follow symbolic links
     if (file.isSymbolicLink()) {
-      // Do not follow symbolic links
+      symbolicLinks.add(f);
     } else if (file.isDirectory()) {
       ArrayPrototypePush(subdirectories, traverse(f, files));
     }
@@ -66,6 +66,7 @@ class FSWatcher extends EventEmitter {
   #options = null;
   #closed = false;
   #files = new SafeMap();
+  #symbolicFiles = new SafeSet();
   #rootPath = path.resolve();
   #watchingFile = false;
 
@@ -112,32 +113,15 @@ class FSWatcher extends EventEmitter {
     }
 
     this.#files.clear();
+    this.#symbolicFiles.clear();
     this.emit('close');
   }
 
-  /**
-   * @param {string} file
-   * @return {string} the full path of `file` if we're watching a directory, or the
-                      filename when watching a file.
-   */
-  #getPath(file) {
-    // Always return the filename instead of the full path if watching a file (not a directory)
-    if (this.#watchingFile) {
-      return StringPrototypeSlice(this.#rootPath, StringPrototypeLastIndexOf(this.#rootPath, '/') + 1);
-    }
-
-    // When watching directories, and root directory is changed, return the full file path
-    if (file === this.#rootPath) {
-      return this.#rootPath;
-    }
-
-    // Otherwise return relative path for all files & folders
-    return path.relative(this.#rootPath, file);
-  }
-
   #unwatchFiles(file) {
     const { unwatchFile } = lazyLoadFsSync();
 
+    this.#symbolicFiles.delete(file);
+
     for (const filename of this.#files.keys()) {
       if (StringPrototypeStartsWith(filename, file)) {
         unwatchFile(filename);
@@ -159,7 +143,11 @@ class FSWatcher extends EventEmitter {
         const f = path.join(folder, file.name);
 
         if (!this.#files.has(f)) {
-          this.emit('change', 'rename', this.#getPath(f));
+          this.emit('change', 'rename', path.relative(this.#rootPath, f));
+
+          if (file.isSymbolicLink()) {
+            this.#symbolicFiles.add(f);
+          }
 
           if (file.isFile()) {
             this.#watchFile(f);
@@ -187,25 +175,28 @@ class FSWatcher extends EventEmitter {
 
     watchFile(file, {
       persistent: this.#options.persistent,
-    }, (statWatcher, previousStatWatcher) => {
+    }, (currentStats, previousStats) => {
       if (existingStat && !existingStat.isDirectory() &&
-        statWatcher.nlink !== 0 && existingStat.mtime?.getTime() === statWatcher.mtime?.getTime()) {
+        currentStats.nlink !== 0 && existingStat.mtimeMs === currentStats.mtimeMs) {
         return;
       }
 
-      this.#files.set(file, statWatcher);
+      this.#files.set(file, currentStats);
 
-      if (statWatcher.birthtimeMs === 0 && previousStatWatcher.birthtimeMs !== 0) {
+      if (currentStats.birthtimeMs === 0 && previousStats.birthtimeMs !== 0) {
         // The file is now deleted
         this.#files.delete(file);
-        this.emit('change', 'rename', this.#getPath(file));
+        this.emit('change', 'rename', path.relative(this.#rootPath, file));
         this.#unwatchFiles(file);
-      } else {
-        this.emit('change', 'change', this.#getPath(file));
-
-        if (statWatcher.isDirectory() && !statWatcher.isSymbolicLink()) {
-          this.#watchFolder(file);
-        }
+      } else if (file === this.#rootPath && this.#watchingFile) {
+        // This case will only be triggered when watching a file with fs.watch
+        this.emit('change', 'change', path.basename(file));
+      } else if (this.#symbolicFiles.has(file)) {
+        // Stats from watchFile does not return correct value for currentStats.isSymbolicLink()
+        // Since it is only valid when using fs.lstat(). Therefore, check the existing symbolic files.
+        this.emit('change', 'rename', path.relative(this.#rootPath, file));
+      } else if (currentStats.isDirectory()) {
+        this.#watchFolder(file);
       }
     });
   }
@@ -224,7 +215,7 @@ class FSWatcher extends EventEmitter {
         this.#files.set(filename, file);
 
         PromisePrototypeThen(
-          traverse(filename, this.#files),
+          traverse(filename, this.#files, this.#symbolicFiles),
           () => {
             for (const f of this.#files.keys()) {
               this.#watchFile(f);

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

@@ -24,19 +24,25 @@ tmpdir.refresh();
 (async () => {
   // Add a recursive symlink to the parent folder
 
-  const testsubdir = fs.mkdtempSync(testDir + path.sep);
-  const file = 'file-1.txt';
-  const filePath = path.join(testsubdir, file);
-  const watcher = fs.watch(testsubdir, { recursive: true });
+  const testDirectory = fs.mkdtempSync(testDir + path.sep);
 
-  const symlinkFile = path.join(testsubdir, 'symlink-folder');
-  fs.symlinkSync(testsubdir, symlinkFile);
+  // Do not use `testDirectory` as base. It will hang the tests.
+  const rootDirectory = path.join(testDirectory, 'test-1');
+  fs.mkdirSync(rootDirectory);
 
+  const filePath = path.join(rootDirectory, 'file.txt');
+
+  const symlinkFolder = path.join(rootDirectory, 'symlink-folder');
+  fs.symlinkSync(rootDirectory, symlinkFolder);
+
+
+  const watcher = fs.watch(rootDirectory, { recursive: true });
   let watcherClosed = false;
   watcher.on('change', function(event, filename) {
-    assert.ok(event === 'change' || event === 'rename');
+    assert.ok(event === 'rename', `Received ${event}`);
+    assert.ok(filename === path.basename(symlinkFolder) || filename === path.basename(filePath), `Received ${filename}`);
 
-    if (filename === file) {
+    if (filename === path.basename(filePath)) {
       watcher.close();
       watcherClosed = true;
     }
@@ -51,36 +57,42 @@ tmpdir.refresh();
 })().then(common.mustCall());
 
 (async () => {
-  // Add a recursive symlink outside the parent folder
+  // This test checks how a symlink to outside the tracking folder can trigger change
+  // tmp/sub-directory/tracking-folder/symlink-folder -> tmp/sub-directory
 
-  const testsubdir = fs.mkdtempSync(testDir + path.sep);
-  const file = 'file-1.txt';
-  const filePath = path.join(testsubdir, file);
+  const rootDirectory = fs.mkdtempSync(testDir + path.sep);
 
-  const trackFolder = path.join(testsubdir, 'tracking-folder');
-  const acceptableFile = 'acceptable-1.txt';
-  const acceptableFilePath = path.join(trackFolder, acceptableFile);
-  fs.mkdirSync(trackFolder);
+  const subDirectory = path.join(rootDirectory, 'sub-directory');
+  fs.mkdirSync(subDirectory);
 
-  const symlinkFile = path.join(trackFolder, 'symlink-folder');
-  fs.symlinkSync(testsubdir, symlinkFile);
+  const trackingSubDirectory = path.join(subDirectory, 'tracking-folder');
+  fs.mkdirSync(trackingSubDirectory);
 
-  const watcher = fs.watch(trackFolder, { recursive: true });
+  const symlinkFolder = path.join(trackingSubDirectory, 'symlink-folder');
+  fs.symlinkSync(subDirectory, symlinkFolder);
+
+  const forbiddenFile = path.join(subDirectory, 'forbidden.txt');
+  const acceptableFile = path.join(trackingSubDirectory, 'acceptable.txt');
+
+  const watcher = fs.watch(trackingSubDirectory, { recursive: true });
   let watcherClosed = false;
   watcher.on('change', function(event, filename) {
-    assert.ok(event === 'change' || event === 'rename');
-    assert.ok(filename === 'symlink-folder' || filename === trackFolder || filename === acceptableFile, `Received filename ${filename}`);
+    // macOS will only change the following events:
+    // { event: 'rename', filename: 'symlink-folder' }
+    // { event: 'rename', filename: 'acceptable.txt' }
+    assert.ok(event === 'rename', `Received ${event}`);
+    assert.ok(filename === path.basename(symlinkFolder) || filename === path.basename(acceptableFile), `Received ${filename}`);
 
-    if (filename === acceptableFile) {
+    if (filename === path.basename(acceptableFile)) {
       watcher.close();
       watcherClosed = true;
     }
   });
 
   await setTimeout(common.platformTimeout(100));
-  fs.writeFileSync(filePath, 'world');
+  fs.writeFileSync(forbiddenFile, 'world');
   await setTimeout(common.platformTimeout(100));
-  fs.writeFileSync(acceptableFilePath, 'acceptable');
+  fs.writeFileSync(acceptableFile, 'acceptable');
 
   process.once('exit', function() {
     assert(watcherClosed, 'watcher Object was not closed');