Skip to content

fix(dev): handle errors in watchChange hook#22188

Merged
sapphi-red merged 6 commits intovitejs:mainfrom
Shohjahon-n:fix/watcher-add-unlink-unhandled-promise
Apr 17, 2026
Merged

fix(dev): handle errors in watchChange hook#22188
sapphi-red merged 6 commits intovitejs:mainfrom
Shohjahon-n:fix/watcher-add-unlink-unhandled-promise

Conversation

@Shohjahon-n
Copy link
Copy Markdown
Contributor

@Shohjahon-n Shohjahon-n commented Apr 7, 2026

What is this PR solving?

The onFileAddUnlink function is async and returns a Promise, but the 'add' and 'unlink' watcher event handlers were calling it without handling the returned Promise:

// Before
watcher.on('add', (file) => {
  onFileAddUnlink(file, false)        // Promise is silently dropped
})
watcher.on('unlink', (file) => {
  onFileAddUnlink(file, true)         // Promise is silently dropped
})

This has three consequences:

  1. Unhandled Promise Rejection — if any of the internal await calls inside onFileAddUnlink (e.g. pluginContainer.watchChange, clientModuleGraph.getModuleByUrl, or onHMRUpdate) throw an error, the rejection is never caught. In Node.js 15+, an unhandled rejection crashes the process, killing the dev server.

  2. Race condition — since the 'change' handler is async and properly awaits its operations, but 'add'/'unlink' are not, concurrent file events can cause operations to run out of order (e.g. moduleGraph invalidation racing with publicFiles update).

  3. Silent HMR failureonHMRUpdate('create'/'delete', file) at the end of onFileAddUnlink may not complete before a subsequent event runs, causing HMR notifications to be missed. This is why users sometimes need to manually restart the dev server after adding or deleting files.

Compare with the 'change' handler, which correctly uses async/await:

watcher.on('change', async (file) => {
  await Promise.all(...)
  await onHMRUpdate('update', file)
})

Fix

Added .catch() on the returned Promise so errors surface through the logger instead of crashing or disappearing silently:

watcher.on('add', (file) => {
  onFileAddUnlink(file, false).catch((e) => server.config.logger.error(e))
})
watcher.on('unlink', (file) => {
  onFileAddUnlink(file, true).catch((e) => server.config.logger.error(e))
})

Tests

Unit tests: 57 test files, 766 tests pass. The fix only affects the error path that was previously silently swallowed — no behavioural change under normal conditions.

@sapphi-red
Copy link
Copy Markdown
Member

Would you add a test throwing an async error in watchChange, hotUpdate, handleHotUpdate? I guess that's what this fix would handle.

@sapphi-red sapphi-red added p2-edge-case Bug, but has workaround or limited in scope (priority) feat: dev dev server labels Apr 8, 2026
@Shohjahon-n
Copy link
Copy Markdown
Contributor Author

Added tests for the watchChange throwing case on both 'add' and 'unlink' events in packages/vite/src/node/__tests__/plugins/hooks.spec.ts.

Regarding hotUpdate and handleHotUpdate: after tracing through the call chain, errors thrown by those hooks are caught internally within handleHMRUpdate and never propagate back up to onFileAddUnlink. Specifically:

  • hotUpdate errors are caught in a try/catch inside handleHMRUpdate, stored temporarily, and then re-thrown inside the hmr() closure where they're caught again and forwarded to the client as an HMR error payload via environment.hot.send({ type: 'error', ... }).
  • handleHotUpdate is only invoked when type === 'update', so it is never called from the 'add'/'unlink' paths at all.

So the only hook whose rejection can actually escape onFileAddUnlink and trigger an unhandled promise rejection is watchChange. The two added tests cover that path.

@sapphi-red sapphi-red changed the title fix(server): handle rejected promise in watcher 'add'/'unlink' handlers fix(dev): handle errors in watchChange hook Apr 9, 2026
sapphi-red
sapphi-red previously approved these changes Apr 9, 2026
Copy link
Copy Markdown
Member

@sapphi-red sapphi-red left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Copy link
Copy Markdown
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also need to handle the error here because it calls watchChange too:

watcher.on('change', async (file) => {
file = normalizePath(file)
reloadOnTsconfigChange(server, file)
await Promise.all(
Object.values(server.environments).map((environment) =>
environment.pluginContainer.watchChange(file, { event: 'update' }),
),
)
// invalidate module graph cache on file change
for (const environment of Object.values(server.environments)) {
environment.moduleGraph.onFileChange(file)
}
await onHMRUpdate('update', file)
})

@sapphi-red sapphi-red merged commit fc08bda into vitejs:main Apr 17, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feat: dev dev server p2-edge-case Bug, but has workaround or limited in scope (priority)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants