fix: hmr with fsCache after concurrent file changes

[patak-dev]

Description

It seems we have a race condition for the fs cached checks when automated refactoring tools are used, or the user changes branches with the dev server open. Issue first reported by @cpojer here

Scenario (with the dev server open):

• A user renames a file (add, unlink events).
• It then modifies the import (change event).
• HMR works properly, the change is processed after add, unlink so the fs cache is already updated.
• git commit
• git checkout previous commit (change, add, unlink events triggered concurrently)
• HMR could break if we process the change event before the rename (add, unlink)

The same could happen with an automated refactoring, depending on the order they do changes.

I don't know how we can support this except by adding debouncing the processing of watch events so we can ensure that the fs cache has time to be updated before we start the HMR processing. This PR adds a 50ms debounce if the fs cache is active.

If this is the only solution to this problem, the tradeoffs of having the fs cache by default changes a bit. We are now trading faster startup and processing for slower HMR. If the needed debounce delay is ~25ms maybe it is acceptable? If we need 50-100ms, then I think we may need to get back to the fs cache being opt-in.

[stackblitz]

Run & review this pull request in StackBlitz Codeflow.

[sapphi-red]

I don't know how we can support this except by...

I guess the ideal fix would be fixing #10663.

[patak-dev]

Oh, that's a great reference @sapphi-red. Thanks for connecting the dots here. I agree. The fs cache is just making this issue more prominent. So even in the worse case of having to temporarily make it opt-in again, we could have opportunities to bring it back in the future.

[sapphi-red]

@patak-dev Do you prefer merging this one as a workaround until #10663 is fixed? If so, I'll review this PR.

[patak-dev]

@sapphi-red I was waiting for feedback from @cpojer regarding the minimum value for the delay. If we need to add a 50ms delay, maybe we should disable the cache by default. We could try merging with a 10ms for now to see if that is enough maybe 🤔

[sapphi-red]

I see. Let's wait for now then 👍

[cpojer]

Ah, apologies, I didn't realize you were waiting for my input on this issue. Any delay that you are choosing here will inevitably lead to race conditions that only some users will run into some of the time. Why can HMR not wait for the filesystem changes directly?

[patak-dev]

Any delay that you are choosing here will inevitably lead to race conditions that only some users will run into some of the time. Why can HMR not wait for the filesystem changes directly?

The issue is that when you run git switch -, there may be N different files change almost at the same time. Each of these files will trigger a change, add, or unlink event. In the scenario in the description, depending what file git writes first, the events order could trigger reprocessing of a file for HMR (because of a change that modifies an import path that was renamed) before the add/unlink event triggers the invalidation of the fs cache. If we wait for something, we should somehow detect that a git command was executed and wait until the command is done. I don't know if we can do that from Vite.

But about the race condition, given that git should write the branch changes quickly, the delay is just serving as a way to order very close events. So if you try this PR and it works well for a 5-10ms delay, then maybe we can move forward with it. If you need to set it to 50ms (like it is now), then I think we will need to make the fs cache opt-in.

[cpojer]

I see what you mean. Let's start with a 15ms delay and see how it goes for everyone?

[patak-dev]

I see what you mean. Let's start with a 15ms delay and see how it goes for everyone?

Any chance you could get this branch locally and try it out with 5ms and 10ms? If we go with 15ms, that extra overhead may be there for a long time.

[cpojer]

I see. Unfortunately I'm traveling for the next two weeks. You could start with 5ms and I can report back after my travels if I'm still running into the same issue.

[patak-dev]

@sapphi-red I'm good merging this with a 5ms delay, let me know what you think

[patak-dev]

/ecosystem-ci run

[vite-ecosystem-ci]

📝 Ran ecosystem CI on 23d1867: Open

suite	result	latest scheduled
astro	❌ failure	✅ success

✅ analogjs, histoire, ladle, laravel, marko, nuxt, previewjs, quasar, qwik, rakkas, remix, sveltekit, unocss, vike, vite-plugin-pwa, vite-plugin-react, vite-plugin-react-pages, vite-plugin-react-swc, vite-plugin-svelte, vite-plugin-vue, vite-setup-catalogue, vitepress, vitest

[patak-dev]

/ecosystem-ci run astro

[vite-ecosystem-ci]

📝 Ran ecosystem CI on 23d1867: Open

suite	result	latest scheduled
astro	❌ failure	✅ success

✅

[patak-dev]

It seems Astro CI in their main is pretty unstable right now: https://github.com/withastro/astro/commits/main/

[patak-dev]

/ecosystem-ci run

[vite-ecosystem-ci]

📝 Ran ecosystem CI on 23d1867: Open

suite	result	latest scheduled
astro	❌ failure	✅ success
sveltekit	❌ failure	✅ success

✅ analogjs, histoire, ladle, laravel, marko, nuxt, previewjs, quasar, qwik, rakkas, remix, unocss, vike, vite-plugin-pwa, vite-plugin-react, vite-plugin-react-pages, vite-plugin-react-swc, vite-plugin-svelte, vite-plugin-vue, vite-setup-catalogue, vitepress, vitest

[sapphi-red]

It seems to be because astro expects the event to happen sync when server.watcher.emit is called.
https://github.com/withastro/astro/blob/0ff5d72c7841e10065d37b43322358f4a48b7e95/packages/astro/test/units/dev/dev.test.js#L101
https://github.com/withastro/astro/blob/0ff5d72c7841e10065d37b43322358f4a48b7e95/packages/astro/test/units/test-utils.js#L118-L125

I'll take a look if I can fix #10663 easily as I checked the related code quite recently by coincidence.

[patak-dev]

@cpojer, #16303 has been released in [email protected]. Let us know if you experience further issues with HMR after changing git branches. Shoutout again to @sapphi-red for implementing and pushing for the right fix here 💚

[cpojer]

Thanks so much. I will report back in case there are issues.