Always recreate the file watcher when rename event occurs

[sheetalkamat]

Sometimes file appears before the callback is processed and that results in watching wrong inode.
So on every rename event we are recreating the file watcher (which was intention of the existing code by checking fileExists but now the presence check merely determines which kind of watcher is created)
Actual change is in 5dfdbdb and bb32906

Apart from this it is also noticed that when files are edited with vim, the rename event occurs for fileNameWithExtension~ so that is handled as if the event has occured for fileNameWithExtension. Fixed in 8e036e5

884678e converts all watch related tests to baselines so its easier to reason about
03fa5b3 adds inode watching test by refactoring how sys does fsWatch so that we can test that functionality
c7bbb97 allows to take map of files instead of array when creating test host for watch
189bbc2 test that shows the rename issue
5dfdbdb bb32906 8e036e5 Actual fix

Fixes #47466 (Repro per #47466 (comment))

[sheetalkamat]

@typescript-bot pack this

[typescript-bot]

Heya @sheetalkamat, I've started to run the tarball bundle task on this PR at 5dfdbdb. You can monitor the build here.

[sheetalkamat]

@MarcCelani-at Can you try the result of build from #48997 (comment) Its modified version of #47482

[typescript-bot]

Hey @sheetalkamat, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/125790/artifacts?artifactName=tgz&fileId=D8F15E5B48BB0BFFF93C0427F62ADCE50BE1C0D583BA711499716928612CD14502&fileName=/typescript-4.7.0-insiders.20220506.tgz"
    }
}

and then running npm install.

---

There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@4.7.0-pr-48997-3".;

[sheetalkamat]

@typescript-bot pack this

[typescript-bot]

Heya @sheetalkamat, I've started to run the tarball bundle task on this PR at bb32906. You can monitor the build here.

[typescript-bot]

Hey @sheetalkamat, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/125822/artifacts?artifactName=tgz&fileId=F76F97DD4CBFC6D4C7E802A3078BBF1E223B33A9F218DDA4B9C19783D8CC9D7402&fileName=/typescript-4.7.0-insiders.20220506.tgz"
    }
}

and then running npm install.

---

There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@4.7.0-pr-48997-6".;

[MarcCelani-at]

I can't verify this change in our mono repo because we use yarn3, which patches typescript at fixed versions. But I will test this on a small project on mac.

[MarcCelani-at]

I'm working on trying to repro this. There's a lot changing in our environment lately (just upgraded from node 12 to node 16) so its taking longer than it should, I think.

[jakebailey]

FWIW you can definitely use the above package with yarn 3; it'll cook up a new patched version for whichever version of TS you install, even if it's not one of the stable release versions.

[jakebailey]

To install PRs, we ask the bot to "pack this", which gives you something you can put into your package.json:

"typescript": "npm:@typescript-deploys/pr-build@4.7.0-pr-48997-3"

See above: #48997 (comment)

[MarcCelani-at]

Sorry, this is taking awhile. We are still on 4.5.4 so I first had to get 4.6.4 working. It turns out this is really difficult in our monorepo environment because of a change that broke tsserver-bridge, details in microsoft/vscode#151245. I'm going to try to get this working without using tsserver-bridge in our monorepo.

[MarcCelani-at]

Okay, I have managed to repro the underlying issue on 4.6.4 in our monorepo without crashing the process from memory usage. Then, I upgraded to npm:@typescript-deploys/pr-build@4.7.0-pr-48997-3, and the issue still repros. I checked the logs and confirmed the correct version of typescript was running, and I confirmed that after the first FileWatcher trigger, it does not trigger again after that. I repro'd by...

1. Open a ts file in VSCode
2. Set the typescript version to workspace version
3. Open logs check version
4. Open a terminal
5. Open a file that is imported in vim. Change the class name of the class that is imported and write with ":wq"
6. Observe a correct error in VSCode on the open file
7. Check logs, verify FileWatcher trigger log line
8. Open the same file that is imported in vim. Change the class name back and write with ":wq"
9. Wait, observe that the error does not go away.
10. Check logs, verify FileWatcher did not trigger again

[MarcCelani-at]

ah it appears I need to test npm:@typescript-deploys/pr-build@4.7.0-pr-48997-6, my mistake. Let me try that now!

[MarcCelani-at]

Ok, tried it again on npm:@typescript-deploys/pr-build@4.7.0-pr-48997-6 and it also doesn't fix the problem. :(

[sheetalkamat]

@MarcCelani-at did you see lines like: ``sysLog:: ${fileOrDirectory}:: Changing watcher to ...` what did they say.. Can you paste logs from that line by cleaning out private file paths to some random file paths

[sheetalkamat]

@typescript-bot pack this

[typescript-bot]

Heya @sheetalkamat, I've started to run the tarball bundle task on this PR at e1ba8a8. You can monitor the build here.

[typescript-bot]

Hey @sheetalkamat, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/126860/artifacts?artifactName=tgz&fileId=EA807235A2786888AED5032158CFFB562CCBE9A5DA8BF41F765E8A47440E67BF02&fileName=/typescript-4.8.0-insiders.20220525.tgz"
    }
}

and then running npm install.

---

There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@4.8.0-pr-48997-21".;

[sheetalkamat]

@amcasey @andrewbranch @jakebailey this is ready for review as @MarcCelani-at confirmed offline that this is working

[jakebailey]

Apart from this it is also noticed that when files are edited with vim, the rename event occurs for fileNameWithExtension~ so that is handled as if the event has occured for fileNameWithExtension.

As in, there's no event for fileNameWithExtension at all? Or just that the fix that deals with inodes will also recheck things if the fileNameWithExtension~ file is renamed? (If tilda tempfile renames weren't working currently, I would be super surprised given that's how a lot of editors perform their saves)

[sheetalkamat]

As in, there's no event for fileNameWithExtension at all? Or just that the fix that deals with inodes will also recheck things if the fileNameWithExtension~ file is renamed?

when event occurs on fileNameWithExtension the relative file name in the event is fileNameWithExtension~ and it is treated as if its fileNameWithExtension, So This will update the watchers correctly (that is recreate watchers which it didnt do previously and hence lost events after that.

(If tilda tempfile renames weren't working currently, I would be super surprised given that's how a lot of editors perform their saves)

We received the reports but never concrete repro to see this. Eg on linux vm the tilda thing never came up as part of events and it was working even before that extra commit. In practice we use fsEvents only for directory watching and file watching happens via polling. User has to explicitly set option to fsEvents for file watching to see this.

[jakebailey]

when event occurs on fileNameWithExtension the relative file name in the event is fileNameWithExtension~ and it is treated as if its fileNameWithExtension, So This will update the watchers correctly (that is recreate watchers which it didnt do previously and hence lost events after that.

I see; I was just trying to think of an edge case where this method wouldn't be good, but I suppose at worst, it means we look at the non-~ an extra time or something.

Eg on linux vm the tilda thing never came up as part of events and it was working even before that extra commit

Just so I understand, do you mean that ~ files just never showed up in any file watcher events at all? Or that bugs related to them didn't appear?

[jakebailey]

Okay, I had a change to review all of the commits individually; the first ones all are cleanups and move to baselines that look good to me.

The "actual fix" stuff at the end also looks reasonable, so I'm okay with this change (if everyone else is confident), pending my small questions above. I don't really know if I should be trying to reproduce the core issue or not here to really verify, or if we can just wait for feedback if this breaks people on nightly.

[sheetalkamat]

When i reproed the scenario - vim edit and save on ubuntu vm, i didnt get the ~ for events .. i always received relativeFileName as fileNameWithExtension. Only in the logs that @MarcCelani-at sent i saw that there were events with suffix ~.

I think its reasonable to try it out and seek feedback, Our default is to watch files using polling and not using file system events.. So when we added watchFile option is when people could do this. We have already put caveat that fsEvents and file watching is not reliable with underlying issues so use at own risk. Over years we have gotten reports of things not working as intended but very few people follow up issues with the logs and that that might explain why we haven't seen these kind of logs before.