[20.14.0][fs] recursive watch on Linux crashes on `.close()`

[AviVahl]

Version

20.14.0

Platform

Linux fedora 6.8.11-300.fc40.x86_64 #1 SMP PREEMPT_DYNAMIC Mon May 27 14:53:33 UTC 2024 x86_64 GNU/Linux

Subsystem

internal/fs/recursive_watch

What steps will reproduce the bug?

1. Create a folder with watch.mjs:

import { watch } from "node:fs";

const watcher = watch(import.meta.dirname, { recursive: true });
watcher.on("change", (eventType, filename) => {
  // console.log(eventType, filename);
});
watcher.on("error", (e) => {
  console.error(e);
});

console.log("Watching for changes...");

2. git init in folder
3. node watch.mjs
4. in the same folder, run the following:

for run in {1..100}; do git add watch.mjs && git rm --cached watch.mjs; done

the above command stages and unstages watch.mjs 100 times. you should see watch errors

Error: ENOENT: no such file or directory, stat '/home/avi/projects/watch-crash/.git/index.lock'
    at statSync (node:fs:1658:25)
    at #watchFile (node:internal/fs/recursive_watch:152:28)
    at #watchFolder (node:internal/fs/recursive_watch:129:26)
    at FSWatcher.<anonymous> (node:internal/fs/recursive_watch:184:26)
    at FSWatcher.emit (node:events:519:28)
    at FSWatcher._handle.onchange (node:internal/fs/watchers:215:12)

and

Error: ENOENT: no such file or directory, watch '/home/avi/projects/watch-crash/.git/index.lock'
    at FSWatcher.<computed> (node:internal/fs/watchers:247:19)
    at watch (node:fs:2469:36)
    at #watchFile (node:internal/fs/recursive_watch:156:21)
    at #watchFolder (node:internal/fs/recursive_watch:129:26)
    at FSWatcher.<anonymous> (node:internal/fs/recursive_watch:184:26)
    at FSWatcher.emit (node:events:519:28)
    at FSWatcher._handle.onchange (node:internal/fs/watchers:215:12)

This is because git saves the lock, and removes it immediately.
Sometimes this statSync call fails (recursive_watch:152):

{
      const existingStat = statSync(file);
      this.#files.set(file, existingStat);
    }

which could have been avoided by using throwIfNoEntry: false and checking the return value.

the real race happens when the above statSync call succeeds, and then the watch() call (two lines down) fails because the file just got deleted. This can be seen in the second stack trace above. In this scenario, this.#files will be populated with the stats object, but this.#watchers won't have a matching watcher registered in the map.

When calling close() on watcher after the second scenario happened, the following loop in close will fail:

    for (const file of this.#files.keys()) {
      this.#watchers.get(file).close();
      this.#watchers.delete(file);
    }

as this.#watchers.get(file).close(); will fail when trying to call close() on undefined (watch() call blew up, so nothing in this.#watchers for file).

the error looks like:

TypeError: Cannot read properties of undefined (reading 'close')
      at FSWatcher.close (node:internal/fs/recursive_watch:86:31)

How often does it reproduce? Is there a required condition?

Pretty consistently.

What is the expected behavior? Why is that the expected behavior?

Don't crash on close().
When querying whether a node exists or not (statSync), use throwIfNoEntry to avoid generated errors.

What do you see instead?

calling close() crashes the process

Additional information

@mcollina you might be interested. looks like your cup of tea.

[jakecastelli]

I am on macOS aarch 64 with node v20.13.1 - I didn't see the same error but I got fatal: pathspec 'indx.mjs' did not match any files when do for run in {1..100}; do git add watch.mjs && git rm --cached watch.mjs; done, I assume I will need to test this on Linux.

Also I added console.count('file has been changed') in the on('change') and I got file has been changed: 207 207 times.

[AviVahl]

The for run in {1..100}; do git add watch.mjs && git rm --cached watch.mjs; done is just a bash one-liner to git stage and unstage watch.mjs 100 times really quickly. This causes the .lock file to be created and deleted quickly many times.

[jakecastelli]

yes, I did run it as a bash one-liner, and that's what I got from the tty fatal: pathspec 'index.mjs' did not match any files

edit: I didn't change the file name in the bash one liner script to index.mjs, after I changed to index.mjs and it works, but it didn't crash on Mac.

[AviVahl]

Not sure where the indx.mjs came from. file has to be named watch.mjs for the steps to work.
Alternatively, I've also reproduce this, and additional full crashes (unhandled exceptions) using:
for run in {1..100}; do git init && rm -rf .git; done

above will initialize a git repo in the current directory and remove it right away 100 times.

make sure you do it all in a new empty directory.

[jakecastelli]

Not sure where the indx.mjs came from

Sorry for the confusion, I renamed watch.mjs to index.mjs on my end, but apart from that everything else should be the same.

I tested for run in {1..100}; do git init && rm -rf .git; done, there is no error this time, but I didn't reproduce the crash, probably because fs implementation in libuv for mac and linux is different.

I tested on version node v20.14 mac m1.

Will see what other people reproduce on this one.

[AviVahl]

I believe It has to be Linux to be using the internal recursive_watch.js (and see the issue)

[mcollina]

Here is a fix: #53452