watch: detect files replaced via unlink and create

Signed-off-by: marcopiraccini <marco.piraccini@gmail.com>

Author: marcopiraccini

lib/internal/watch_mode/files_watcher.js

@@ -120,7 +120,9 @@ class FilesWatcher extends EventEmitter {
     watcher.on('change', (eventType, fileName) => {
       // `fileName` can be `null` if it cannot be determined. See
       // https://github.com/nodejs/node/pull/49891#issuecomment-1744673430.
-      this.#onChange(recursive ? resolve(path, fileName ?? '') : path, eventType);
+      // `path` is the watched directory (see `filterFile`), so resolve the
+      // changed entry against it to get the absolute path of the trigger.
+      this.#onChange(resolve(path, fileName ?? ''), eventType);
     });
     this.#watchers.set(path, { handle: watcher, recursive });
     if (recursive) {
@@ -133,9 +135,13 @@ class FilesWatcher extends EventEmitter {
     if (supportsRecursiveWatching) {
       this.watchPath(dirname(file), true, options);
     } else {
-      // Having multiple FSWatcher's seems to be slower
-      // than a single recursive FSWatcher
-      this.watchPath(file, false, options);
+      // Watch the parent directory non-recursively rather than the file
+      // itself. A watch bound to the file's inode stops receiving events once
+      // the file is replaced via unlink+create or rename (atomic saves, Docker
+      // Compose watch, ...), so only the first replacement would be detected.
+      // Watching the directory keeps working across replacements, and unrelated
+      // siblings are discarded by the `filter` mode check in `#onChange`.
+      this.watchPath(dirname(file), false, options);
     }
     this.#filteredFiles.add(file);
     if (owner) {

test/parallel/test-watch-mode-files_watcher.mjs

@@ -6,7 +6,7 @@ import path from 'node:path';
 import assert from 'node:assert';
 import process from 'node:process';
 import { describe, it, beforeEach, afterEach } from 'node:test';
-import { writeFileSync, mkdirSync, appendFileSync } from 'node:fs';
+import { writeFileSync, mkdirSync, appendFileSync, rmSync } from 'node:fs';
 import { createInterface } from 'node:readline';
 import { setTimeout } from 'node:timers/promises';
 import { once } from 'node:events';
@@ -44,6 +44,19 @@ describe('watch mode file watcher', () => {
     });
   }
 
+  function replaceAndWaitForChanges(watcher, file) {
+    return new Promise((resolve) => {
+      const interval = setInterval(() => {
+        rmSync(file, { force: true });
+        writeFileSync(file, `replace ${counter++}`);
+      }, 100);
+      watcher.once('changed', () => {
+        clearInterval(interval);
+        resolve();
+      });
+    });
+  }
+
   it('should watch changed files', async () => {
     const file = tmpdir.resolve('file1');
     writeFileSync(file, 'written');
@@ -52,6 +65,19 @@ describe('watch mode file watcher', () => {
     assert.strictEqual(changesCount, 1);
   });
 
+  it('should keep detecting files replaced via unlink and create', async () => {
+    // Regression test for https://github.com/nodejs/node/issues/51621: a watch
+    // bound to the file inode stops firing after the first replacement, so the
+    // second `replaceAndWaitForChanges` call would hang on the buggy behavior.
+    const file = tmpdir.resolve('replaced.js');
+    writeFileSync(file, 'written');
+    watcher.filterFile(file);
+    await replaceAndWaitForChanges(watcher, file);
+    await replaceAndWaitForChanges(watcher, file);
+    await replaceAndWaitForChanges(watcher, file);
+    assert.ok(changesCount >= 3, `expected at least 3 changes, got ${changesCount}`);
+  });
+
   it('should watch changed files with same prefix path string', async () => {
     mkdirSync(tmpdir.resolve('subdir'));
     mkdirSync(tmpdir.resolve('sub'));
@@ -204,10 +230,9 @@ describe('watch mode file watcher', () => {
     const child = spawn(process.execPath, [file], { stdio: ['pipe', 'pipe', 'pipe', 'ipc'], encoding: 'utf8' });
     watcher.watchChildProcessModules(child);
     await once(child, 'exit');
-    let expected = [file, tmpdir.resolve('file')];
-    if (supportsRecursiveWatching) {
-      expected = expected.map((file) => path.dirname(file));
-    }
+    // The parent directory is watched on every platform so that files replaced
+    // via unlink+create or rename are still detected.
+    const expected = [file, tmpdir.resolve('file')].map((file) => path.dirname(file));
     assert.deepStrictEqual(watcher.watchedPaths, expected);
   });
 });