From 5e61a8c5b0aadc66afc69ede3a9fa9b9150fe21e Mon Sep 17 00:00:00 2001 From: Sameer Ahmed <166151464+codeNinja62@users.noreply.github.com> Date: Fri, 16 Oct 2026 17:48:45 +0500 Subject: [PATCH] fs: report errors from recursive watch events On Linux, the recursive fs.watch() fallback resolves inotify events with stat() and lstat() calls, and arms fs.watch() on directories and symbolic links that appear later. Errors from those calls other than ENOENT were thrown from inside the FSEvent callback, where nothing can catch them, so the process exited: EACCES when a watched directory became unreadable, or when a new directory or the target of a new symbolic link was unreadable by the time it was handled; ELOOP for a symbolic link loop; and ENOTDIR when a watched directory was replaced by a file while events naming its entries were still queued. Report such errors through 'error', as #scanFolder() already does for readdir(), and take ENOTDIR from lstat() to mean that the entry is gone, as statSync() and #scanFolder() do. A directory or link that cannot be watched is dropped, so a later event about it picks it up again. Failures during the initial scan still propagate out of fs.watch(). Fixes: https://github.com/nodejs/node/issues/66791 Signed-off-by: Sameer Ahmed <166151464+codeNinja62@users.noreply.github.com> --- lib/internal/fs/recursive_watch.js | 68 ++++++++++++--- ...fs-watch-recursive-dir-replaced-by-file.js | 35 ++++++++ .../test-fs-watch-recursive-symlink-loop.js | 37 +++++++++ .../test-fs-watch-recursive-unreadable-dir.js | 82 +++++++++++++++++++ 4 files changed, 209 insertions(+), 13 deletions(-) create mode 100755 test/parallel/test-fs-watch-recursive-dir-replaced-by-file.js create mode 100755 test/parallel/test-fs-watch-recursive-symlink-loop.js create mode 100644 test/parallel/test-fs-watch-recursive-unreadable-dir.js diff --git a/lib/internal/fs/recursive_watch.js b/lib/internal/fs/recursive_watch.js index 85d84f9dd8f3..cb2269425506 100644 --- a/lib/internal/fs/recursive_watch.js +++ b/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,18 +237,21 @@ 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; @@ -249,7 +259,14 @@ class FSWatcher extends EventEmitter { 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); } } diff --git a/test/parallel/test-fs-watch-recursive-dir-replaced-by-file.js b/test/parallel/test-fs-watch-recursive-dir-replaced-by-file.js new file mode 100755 index 000000000000..23ba12b2fdd4 --- /dev/null +++ b/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'), ''); diff --git a/test/parallel/test-fs-watch-recursive-symlink-loop.js b/test/parallel/test-fs-watch-recursive-symlink-loop.js new file mode 100755 index 000000000000..74f56fdc2bad --- /dev/null +++ b/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'), ''); diff --git a/test/parallel/test-fs-watch-recursive-unreadable-dir.js b/test/parallel/test-fs-watch-recursive-unreadable-dir.js new file mode 100644 index 000000000000..6c2e9cd1c657 --- /dev/null +++ b/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());