feat: skip soft invalidation error when timestamp mismatch

[antfu]

Description

This check will be hit on a racing condition that invalidates module invalidateModule being called while the project is still warming up:

vite/packages/vite/src/node/server/transformRequest.ts

Lines 403 to 407 in 8f5cdc8

	if (ssr ? mod.ssrTransformResult : mod.transformResult) {
	throw new Error(
	`Internal server error: Soft-invalidated module "${mod.url}" should not have existing transform result`,
	)
	}

This happens on UnoCSS because UnoCSS has a virtual module, uno.css, that serves to generate CSS based on the scanned source code. Meaning that it will self-invalidate whenever a new module is scanned (via the transform hook). In that case, it could cause the entry module (that imports uno.css into a soft invalidate state before its transforming process finishes).

Linked Issue: fix unocss/unocss#3472

This PR fixes the racing condition by resetting the InvalidationState state on the new transformResult set.

---

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

Before submitting the PR, please make sure you do the following

• Read the Contributing Guidelines, especially the Pull Request Guidelines.
• Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
• Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
• Update the corresponding documentation if needed.
• Ideally, include relevant tests that fail without this PR but pass with it.

[stackblitz]

Run & review this pull request in StackBlitz Codeflow.

[patak-dev]

Sounds good to me. I'll let @bluwy have the opportunity to check this one too as he was implementing the soft invalidation logic. Maybe we could remove these two lines after this PR:

vite/packages/vite/src/node/server/transformRequest.ts

Lines 397 to 398 in 8f5cdc8

	if (ssr) mod.ssrInvalidationState = undefined
	else mod.invalidationState = undefined

That could save some invalidations when the result is not actually used:

vite/packages/vite/src/node/server/transformRequest.ts

Line 469 in 8f5cdc8

server.moduleGraph.updateModuleTransformResult(mod, result, ssr)

[patak-dev]

Note: Windows is failing in every PR, ignore that

[bluwy]

I remember intentionally not resetting the invalidationState when the module is updated, because I only want it to reset after we handle it. So I'd like to figure out the issue in the first place 🤔

Currently our code contains this:

vite/packages/vite/src/node/server/transformRequest.ts

Lines 466 to 469 in 734a9e3

	// Only cache the result if the module wasn't invalidated while it was
	// being processed, so it is re-processed next time if it is stale
	if (timestamp > mod.lastInvalidationTimestamp)
	server.moduleGraph.updateModuleTransformResult(mod, result, ssr)

Which implies that we only update the result if the module had not been invalidated. But if it hasn't invalidated, shouldn't invalidationState also be null?

Could the culprit here be the difference between lastHMRTimestamp and lastInvalidationTimestamp? Do you know how invalidateModule is being called? (whether isHmr parameter is passed) Perhaps it had invalidated with isHmr: true that updates lastHMRTimestamp only, and that's why in the code above we thought that the module had not been invalidated yet because we're only checking lastInvalidationTimestamp.

lastHMRTimestamp traces back to 0974dbe#diff-54add59d921f03d4386f04ed8726faa5634abb4898f3e5a14f2c641431475781 and lastInvalidationTimestamp traces back to #7254. Maybe we need to unify them (unless I miss how the differ).

I can take a deeper look in the repro too later tonight.

---

I can understand that this PR is probably the quicker fix, and we can do that if it's urgent though.

[antfu]

Here is how UnoCSS invalidates itself:
https://github.com/unocss/unocss/blob/ec77c7f75343859180a51f1694342219af87b112/packages/vite/src/modes/global/dev.ts#L45-L59
Passed ids are:
https://github.com/unocss/unocss/blob/ec77c7f75343859180a51f1694342219af87b112/packages/vite/src/modes/global/dev.ts#L106-L108
Which is usually just ['/___uno.css']

In Nuxt, entry.js is the root entry. When invaliding the unocss module, it marks entry.js as a soft invalidate. And somehow cause the errors.

I tried to patch Vite to log more, here is what I got (without this PR):

[bluwy]

Thanks! I guess my hunch is incorrect then, but the issue now looks even stranger.

In that case, it could cause the entry module (that imports uno.css into a soft invalidate state before its transforming process finishes).

Trying to think of the scenarios, but I still can't wrap my head around how this issue hits. moduleGraph.invalidateModule would always clear out mod.transformResult, so while entry.js was transforming, depending on whether the invalidation happens before/after we check for soft-invalidation for entry.js, it seems impossible to be able to assign mod.transformResult with mod.invalidationState not cleared yet. I tried to run the repro linked, but I can't get it to reproduce locally.

I don't quite get the problem yet, but as another proposal fix, maybe we can skip the error if timestamp < mod.lastInvalidationTimestamp? I only added the error to make sure we shouldn't hit this state if we refactored the code in the future, so it's not really crucial to make things work.

[antfu]

Honestly, to me, it's also a bit hard to debug as there are several moving parts together. In Nuxt we have SSR and two Vite servers, not entirely sure why this would occur. But from what I am logging, I see transformResult get cleaned out correctly after I call invalidate, but at the time handleModuleSoftInvalidation gets called, the transformResult has been fulfilled and causes the error.

(oh by the way, in the unocss reproduction, the error only occurs on first hard-refresh, not sure if it help with the context).

I wonder if we could just remove this hard throw?

vite/packages/vite/src/node/server/transformRequest.ts

Lines 403 to 407 in 8f5cdc8

	if (ssr ? mod.ssrTransformResult : mod.transformResult) {
	throw new Error(
	`Internal server error: Soft-invalidated module "${mod.url}" should not have existing transform result`,
	)
	}

I guess it was for defensive programming? But with our transform logics to be quite many segments of async chunks, I feel it's hard to guarantee the two states won't co-exist.

[bluwy]

I'm pretty sure there's no way the two states can co-exist from what I can try to think of. Unless something external is mutating Vite's module nodes, e.g. manually assigning transformResult. Does Nuxt do something like that? The two Vite servers could also be the issue, but I think the states should be isolated.

The error is indeed defensive programming though (I mentioned the reason in the last paragraph here). If the error happens, then something unusual is going on, and it implies that "the module has existing transformResult, but we're going to soft-invalidate and re-assign transformResult to something different", which can be unintentional and cause bugs in the future.

I'm ok with skipping the error only if the timestamp < mod.lastInvalidationTimestamp (from my previous comment) though, since the transformResult is not going to be saved anyway. Curious if this will work for your case.

(EDIT: Ah yeah I can see the issue now with a hard refresh, let me take a closer look 😄)

[antfu]

I don't think either Nuxt or UnoCSS manipulates transformResult.

I tried to log it a bit more, if I put:

The track trace suggest it's triggered by these lines:

vite/packages/vite/src/node/server/transformRequest.ts

Lines 364 to 367 in 734a9e3

	// Only cache the result if the module wasn't invalidated while it was
	// being processed, so it is re-processed next time if it is stale
	if (timestamp > mod.lastInvalidationTimestamp)
	moduleGraph.updateModuleTransformResult(mod, result, ssr)

---

I also tried skipping the error with timestamp < mod.lastInvalidationTimestamp and it seems to work in my case. I am happy to go in that direction :)

[antfu]

Updated the PR implementation

[bluwy]

I also tried skipping the error with timestamp < mod.lastInvalidationTimestamp and it seems to work in my case. I am happy to go in that direction :)

The current code seems to be calling the error if timestamp < mod.lastInvalidationTimestamp though 😅 I guess the workaround might not work then.

---

I checked the repro and understand the issue better now. On the request that was assigning transformResult while the module was in an soft-invalidated state, it can happen because initially:

vite/packages/vite/src/node/server/transformRequest.ts

Lines 141 to 147 in 775bb50

	const module = await server.moduleGraph.getModuleByUrl(url, ssr)
	// tries to handle soft invalidation of the module if available,
	// returns a boolean true is successful, or false if no handling is needed
	const softInvalidatedTransformResult =
	module &&
	(await handleModuleSoftInvalidation(module, ssr, timestamp, server))

module is undefined because the module via the url does not exist yet. But after we processed until here:

vite/packages/vite/src/node/server/transformRequest.ts

Lines 283 to 284 in 775bb50

	// ensure module in graph after successful load
	mod ??= await moduleGraph._ensureEntryFromUrl(url, ssr, undefined, resolved)

The module returned from resolveEntryFromUrl was actually an existing module that has been soft-invalidated, because the URLs have the same resolvedId, so that's how the whole issue started.

---

This is interesting because it revealed a potential path we can optimize. After we resolve the id here:

vite/packages/vite/src/node/server/transformRequest.ts

Lines 161 to 163 in 775bb50

	const resolved = module
	? undefined
	: (await pluginContainer.resolveId(url, undefined, { ssr })) ?? undefined

The module may already exists in the module graph and had been soft invalidated, so we only need to handle the soft invalidation instead of going through the whole load+transform phase. Basically, we can copy the above "memory" handling but instead of getModuleByUrl, we use getModuleById.

I could try to implement this and see if it helps

[bluwy]

I'll close this for now since #15785 should fix it. Thanks for kicking this off!