feat(dev): detect 'use cache' module-scope deadlocks early in dev (#1126)

[Divkix]

Summary

Implements a dev-only probe that detects when a 'use cache' fill is stuck on module-scope shared state (e.g. a top-level Map used to dedupe fetches), matching Next.js behavior from vercel/next.js#93500 and #93538.

How it works

• Error classes: UseCacheTimeoutError and UseCacheDeadlockError with digest fields for type-safe detection.
• Probe scheduler: In dev mode, after 10s of idle cache-fill stream time, schedules a probe.
• Probe pool: Fresh Vite ModuleRunner instances with isolated EvaluatedModules graphs, so top-level module state is recreated from scratch.
• Request store snapshot: Forwards cookies, headers, draftMode, and rootParams so private caches behave correctly in the probe.
• Mid-probe recovery: If the main stream emits chunks while the probe runs, the probe result is discarded (no false positives).
• HMR teardown: The probe pool is torn down on file invalidation so probes always use fresh code.
• Production safe: All probe code is gated on process.env.NODE_ENV === "development" and tree-shakes out of production builds.

Test coverage

• Unit tests for error classes and probe scheduler behavior.
• Fixture pages: use-cache-deadlock (never-resolving module-scope promise) and use-cache-recovery (slow-but-working I/O).

Files changed

• packages/vinext/src/shims/use-cache-errors.ts (new)
• packages/vinext/src/shims/use-cache-probe-globals.ts (new)
• packages/vinext/src/server/use-cache-probe-scheduler.ts (new)
• packages/vinext/src/server/use-cache-probe-pool.ts (new)
• packages/vinext/src/server/dev-module-runner.ts
• packages/vinext/src/shims/cache-runtime.ts
• packages/vinext/src/index.ts
• tests/shims.test.ts
• tests/fixtures/app-basic/app/use-cache-deadlock/page.tsx (new)
• tests/fixtures/app-basic/app/use-cache-recovery/page.tsx (new)

Closes #1126

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/vinext@1157

commit: 0b10e4f

[james-elicx]

/bigbonk review

[ask-bonk]

Review: use cache deadlock probe

The overall direction — detect module-scope deadlocks in dev by re-running the cache function in an isolated ModuleRunner — makes sense and mirrors the Next.js approach well. However, there are several issues that need to be addressed before this can merge.

Critical

1. runWithProbeRequestStore uses native import() instead of runner.import(). The entire point of the probe pool is that it runs in an isolated module graph. But runWithProbeRequestStore (lines 189-193 of use-cache-probe-pool.ts) uses native import("vinext/shims/...") — which resolves to the main process's module instances. This means the ALS context, headers, and request context are set on the main runner's modules, not the probe runner's. The probe's fn() runs with the main-runner's ALS, defeating the isolation guarantee. These imports should go through the probe runner via runner.import().

2. use-cache-probe-scheduler.ts is dead code. It is never imported by any production code — only by the test file. The actual probe logic was inlined directly into cache-runtime.ts (lines 403-467). This 173-line file ships in the package for no reason. Either use it (by having cache-runtime.ts delegate to it) or remove it.

3. Timer leak on successful execution. In cache-runtime.ts, both the 10s probe setTimeout (line 429) and the 54s timeout setTimeout (line 452) are never cleared when executionPromise wins the Promise.race. The 10s timer will still fire 10 seconds later and invoke the probe on an already-completed function. The 54s timer is .unref()'d so it won't keep the process alive, but the 10s probe timer is not unref'd and will fire unconditionally. Both timers should be cleared when execution completes.

Moderate

4. _environment is stored but never read. The module-scope variable _environment in use-cache-probe-pool.ts is assigned on init (line 52) and nulled on teardown (line 164), but is never used anywhere. It should be removed.

5. Redundant await after runWithProbeRequestStore already awaited. In use-cache-probe-pool.ts lines 123-141, runWithProbeRequestStore is awaited (assigning to result), then result is passed through Promise.resolve(result) in another Promise.race. If the function already completed via await, the second Promise.race with a timeout is a no-op — result is already resolved. The timeout race should wrap the un-awaited call, or this entire section should be restructured.

6. Round-robin pool reuses stale module state. The pool creates 4 runners at init and round-robins them. But the whole point is to get fresh module-scope state. The second time a runner is used, its evaluated modules cache already has the cached function's module loaded with the previous run's top-level state. Either create a fresh runner per probe (and dispose it afterward), or call runner.close() + recreate between uses.

7. benchmarks/vinext/tsconfig.json change is unrelated. Adding a vinext path mapping to the benchmarks tsconfig is not related to the deadlock probe feature. If it's needed for a separate reason, it should be a separate commit/PR.

Minor

8. Fixture pages were added then removed in follow-up commits. The PR description mentions use-cache-deadlock and use-cache-recovery fixture pages but commit 4b2c506 removed them because they caused CI timeouts. This means the feature has no integration-level test coverage — only unit tests for the error classes and the scheduler's bail-out paths. The deadlock detection path itself (probe completes → deadlock error raised) has zero test coverage.

9. Probe encodedArguments uses JSON.stringify which loses non-serializable values. The probe falls back to JSON.stringify(args) (line 436 in cache-runtime.ts, line 114 in use-cache-probe-pool.ts). For the stated purpose ("exact argument values matter less than the fact that the function body executes"), this is acknowledged. But if the function reads its arguments (e.g., uses a passed-in object reference), the JSON round-trip could cause the probe to succeed where the real call fails (false positive deadlock detection). Worth documenting this limitation more explicitly.

[ask-bonk]

Critical: These import() calls use the host process's native module resolution — they load the main runner's module instances, not the probe runner's isolated graph. The comment says "These imports run inside the probe runner's module graph" but that's not what happens with native import().

To actually run in the probe's isolated graph, these would need to go through runner.import("vinext/shims/unified-request-context") and runner.import("vinext/shims/headers"). But the function doesn't have access to the runner — it would need to be passed in or this logic restructured.

As-is, the ALS context is set on the main runner's modules, so cookies()/headers() inside the probe function will read from the main runner's ALS, not the isolated one. This defeats the purpose of request-store reconstruction.

[ask-bonk]

Timer leak: When executionPromise wins the Promise.race (the normal fast path), this 10s setTimeout is never cleared. It will fire 10 seconds later and invoke the full probe machinery — creating a fresh ModuleRunner import, running the cached function again — on an already-completed call. This is wasteful and could cause confusing log output or errors.

The probePromise.catch(() => {}) swallows the rejection, but the probe itself still runs. You need to clear the timer when execution completes:

Suggested change

	probePromise = new Promise<never>((_, reject) => {
	setTimeout(() => {
	const probeInternalTimeoutMs = fillDeadlineAt - performance.now() - 1_000;
	if (probeInternalTimeoutMs <= 0) return;
	probe({
	id,
	kind: cacheVariant,
	encodedArguments: JSON.stringify(args),
	request: requestSnapshot,
	timeoutMs: probeInternalTimeoutMs,
	}).then(
	(completed) => {
	if (completed) reject(deadlockError);
	},
	() => {},
	);
	}, 10_000);
	});
	// Swallow rejection when execution wins the race.
	probePromise.catch(() => {});
	let probeTimer: ReturnType<typeof setTimeout> | undefined;
	probePromise = new Promise<never>((_, reject) => {
	probeTimer = setTimeout(() => {
	const probeInternalTimeoutMs = fillDeadlineAt - performance.now() - 1_000;
	if (probeInternalTimeoutMs <= 0) return;
	probe({
	id,
	kind: cacheVariant,
	encodedArguments: JSON.stringify(args),
	request: requestSnapshot,
	timeoutMs: probeInternalTimeoutMs,
	}).then(
	(completed) => {
	if (completed) reject(deadlockError);
	},
	() => {},
	);
	}, 10_000);
	});
	// Swallow rejection when execution wins the race.
	probePromise.catch(() => {});

Then after Promise.race resolves, clear both timers.

[ask-bonk]

Dead store: _environment is assigned on line 52 and nulled on line 164, but is never read anywhere. Remove it.

[ask-bonk]

Redundant await + race: runWithProbeRequestStore is awaited on line 123, which means result is already a resolved value by line 125. The Promise.race on lines 132-141 is racing an already-resolved Promise.resolve(result) against a timeout — the resolved promise always wins instantly. This timeout never fires.

The timeout should wrap the un-awaited call:

const result = await Promise.race([
  runWithProbeRequestStore(request, async () => wrapped(...args)),
  new Promise<never>((_, reject) => {
    const t = setTimeout(() => reject(new UseCacheTimeoutError()), remaining);
    if (typeof t.unref === 'function') t.unref();
  }),
]);

[ask-bonk]

Stale module state on reuse: The pool round-robins across 4 pre-created runners, but ModuleRunner caches evaluated modules internally. The second time a runner is used for the same function, the module's top-level scope has already been evaluated — the Maps, Promises, etc. that the probe is supposed to test with fresh state are already initialized from the previous probe run.

This means the first probe for a given function works correctly, but subsequent probes on the same runner reuse stale module state and may not detect the deadlock.

Either:

1. Create a fresh runner per probe and close() it after, or
2. Call runner.close() and replace the runner in the pool after each use

[ask-bonk]

Dead code: setupProbeScheduler is never imported by any production code. The probe logic was inlined directly into cache-runtime.ts instead. This entire 173-line file ships in the package but is only used in tests.

Either:

1. Have cache-runtime.ts delegate to this scheduler (which has better structure — idle-stream tracking, mid-probe recovery via chunk monitoring), or
2. Remove this file entirely

[ask-bonk]

This change is unrelated to the deadlock probe feature. If the benchmarks tsconfig needs a vinext path mapping, that should be a separate commit or PR.

[ask-bonk]

This type change (unknown | undefined → unknown) is unrelated to the deadlock probe feature. unknown | undefined does simplify to unknown so this is technically a no-op, but it should be in a separate commit.

[ask-bonk]

Review submitted. Here's a summary of the findings:

3 critical issues:

1. runWithProbeRequestStore uses native import() instead of the probe runner's runner.import(), meaning the ALS/request context is set on the main runner's modules — completely defeating the isolation purpose
2. use-cache-probe-scheduler.ts is 173 lines of dead code (never imported by production code)
3. Timer leak — the 10s probe timer and 54s timeout timer are never cleared when execution completes normally, causing the probe to fire on already-completed calls

4 moderate issues:
4. _environment variable stored but never read (dead store)
5. Redundant await + Promise.race pattern where the timeout can never fire
6. Round-robin pool reuses runners with stale module state, defeating the fresh-state guarantee after the first probe
7. Unrelated changes to benchmarks/vinext/tsconfig.json and request-pipeline.ts

1 minor issue:
8. No integration test coverage for the actual deadlock detection path (fixture pages were added then removed due to CI timeouts)

github run

[james-elicx]

/bigbonk review

[ask-bonk]

Review: use cache deadlock probe (v2)

Significant progress since the last round — the dead-code scheduler file was removed, unrelated changes cleaned up, the pool was changed from round-robin to fresh-runner-per-probe (good), and timer cleanup was added in the .finally() block. The overall architecture (fresh ModuleRunner per probe, Symbol.for on globalThis for cross-graph handoff) is sound.

However, there are still several issues that need fixing before this can merge.

Critical

1. Probe permanently disabled after first HMR invalidation. tearDownUseCacheProbePool() nulls _probeEnvironment and clears the probe function via setUseCacheProbe(undefined). But initUseCacheProbePool() is only called once — in configureServer() — and is never called again after HMR. The invalidateAppRoutingModules() function calls tearDownUseCacheProbePool() on every file change, so after the very first edit in dev mode, the probe is permanently gone. The comment says "the next probe starts with fresh code" but there's no code path to re-initialize. This needs to either (a) re-init in invalidateAppRoutingModules() after teardown, or (b) restructure teardown to only clear the runners/state without unsetting the probe function (since the environment persists).

2. Recursive probe via shared globalThis. Symbol.for("vinext.dev.useCacheProbe") is visible to all ModuleRunner instances in the same Node.js process. When the probe runner imports cache-runtime.ts and calls registerCachedFunction, that code path hits the isDev branch, calls getUseCacheProbe(), finds the probe function on globalThis, and schedules another 10s probe timer — creating a recursive probe chain. The timeout arithmetic will eventually bail out (probeInternalTimeoutMs <= 0), but until then you're creating unnecessary ModuleRunner instances and timers. The probe runner's cache-runtime should not see the probe function. Either (a) unset the probe on globalThis before calling the wrapped function and restore it after, or (b) add a flag/guard so the probe-within-probe path is skipped.

Moderate

3. createProbeRunner is an exact copy of createDirectRunner. The two functions are identical — same transport, same evaluator, same options. The only difference is the JSDoc comment. Just reuse createDirectRunner; the fact that each call creates a new ModuleRunner (with its own EvaluatedModules) already gives you isolation. There's no need for a separate function.

4. ModuleRunner import is unused as a value. Line 22 imports ModuleRunner from vite/module-runner, but it's only used as a type annotation on line 152 (runner: ModuleRunner). This should be import type { ModuleRunner } to avoid pulling the module at runtime unnecessarily.

5. Error classes don't call super() with the right name. Neither UseCacheTimeoutError nor UseCacheDeadlockError sets this.name. By default, Error subclasses inherit this.name = "Error", so stack traces show Error: Filling a cache... instead of UseCacheTimeoutError: Filling a cache.... Add this.name = "UseCacheTimeoutError" (or the constructor name) in each constructor.

Minor

6. No integration test coverage for the actual deadlock detection. The tests only cover the error classes (digest values, message content). The core behavior — probe completes → deadlock error raised, probe times out → timeout error raised, fast execution → no probe fires — has zero test coverage. The fixture pages were removed due to CI timeouts, which is understandable, but a unit-level test that mocks the probe function and verifies the Promise.race behavior in cache-runtime.ts would be valuable and wouldn't have timeout issues.

7. isDraftMode and isHmrRefresh are always hardcoded to false. In cache-runtime.ts lines 426-427, both fields are hardcoded. If these are genuinely never needed for the probe, they should be removed from UseCacheProbeRequestSnapshot to avoid confusion. If they might be needed later, add a TODO comment.

[ask-bonk]

Critical: probe permanently disabled after first HMR. tearDownUseCacheProbePool() nulls _probeEnvironment and clears the probe function. But initUseCacheProbePool() is only called once (line 2146), not after each teardown. After the first file change, the probe is permanently gone.

Either re-initialize here:

Suggested change

	tearDownUseCacheProbePool();
	tearDownUseCacheProbePool();
	// Re-initialize so probes continue working after HMR.
	const rscEnv = server.environments["rsc"];
	if (rscEnv) {
	initUseCacheProbePool(rscEnv);
	}

Or restructure tearDownUseCacheProbePool so it doesn't unset the probe function — since the environment reference is the same, you could just trust that fresh runners (created per-probe) already give you fresh code.

[ask-bonk]

Critical: recursive probe via shared globalThis. getUseCacheProbe() reads from globalThis[Symbol.for(...)], which is shared across all ModuleRunner instances. When the probe runner re-evaluates cache-runtime.ts and calls registerCachedFunction → getUseCacheProbe(), it sees the same probe function and schedules another 10s timer, spawning yet another ModuleRunner.

The recursion eventually terminates because probeInternalTimeoutMs shrinks to ≤0, but this creates unnecessary runners and timers.

Fix: guard against recursive probing. Simplest approach — add a flag:

let _insideProbe = false;

and in the probe callback (use-cache-probe-pool.ts), set _insideProbe = true before calling the wrapped function (or temporarily unset the probe on globalThis).

[ask-bonk]

Moderate: exact duplicate of createDirectRunner. This function is identical to createDirectRunner above — same transport, same evaluator, same options. Each call already creates a fresh ModuleRunner with its own EvaluatedModules, so there's no need for a separate function.

Just reuse createDirectRunner in use-cache-probe-pool.ts. The JSDoc distinction ("for probes") can be a comment at the call site.

[ask-bonk]

ModuleRunner is only used as a type annotation (line 152). Use a type-only import to avoid pulling the module at runtime:

Suggested change

	import { ModuleRunner } from "vite/module-runner";
	import type { ModuleRunner } from "vite/module-runner";

[ask-bonk]

Neither error class sets this.name. Stack traces will show Error: Filling a cache... instead of UseCacheTimeoutError: Filling a cache....

Suggested change

	'Filling a cache during prerender timed out, likely because request-specific arguments such as params, searchParams, cookies() or dynamic data were used inside "use cache".',
	'Filling a cache during prerender timed out, likely because request-specific arguments such as params, searchParams, cookies() or dynamic data were used inside "use cache".',
	);
	this.name = 'UseCacheTimeoutError';

Same for UseCacheDeadlockError.

[ask-bonk]

Minor: isDraftMode and isHmrRefresh are hardcoded to false and never read from the actual request context. If these are intentionally unused, either remove them from UseCacheProbeRequestSnapshot or add a comment explaining why they're always false (e.g., "not relevant for deadlock detection").

[ask-bonk]

Review submitted. Here's a summary of the findings:

2 critical issues:

1. Probe permanently disabled after first HMR invalidation — tearDownUseCacheProbePool() is called on every file change but initUseCacheProbePool() is only called once at startup, so deadlock detection is permanently gone after the first edit
2. Recursive probe via shared globalThis — Symbol.for("vinext.dev.useCacheProbe") is visible to all ModuleRunner instances, so the probe runner's cache-runtime will find the probe function and schedule its own nested probe, creating a recursive chain

3 moderate issues:
3. createProbeRunner is an exact copy of createDirectRunner — just reuse the existing function
4. ModuleRunner imported as a value but only used as a type annotation
5. Error classes don't set this.name, so stack traces show generic Error: prefix

2 minor issues:
6. No integration test coverage for the actual deadlock detection path
7. isDraftMode and isHmrRefresh hardcoded to false in UseCacheProbeRequestSnapshot but never explained

github run

[james-elicx]

/bigbonk review

[ask-bonk]

Review: use cache deadlock probe (v3)

Good progress from the prior rounds — the previous critical issues (dead scheduler file, native import() vs runner.import(), round-robin pool reuse, timer leaks in the outer cache-runtime.ts) have all been addressed. The architecture is now sound: fresh ModuleRunner per probe, isInsideUseCacheProbe() guard to prevent recursion, timer cleanup via .finally(), and proper HMR teardown+reinit.

Two issues remain that should be fixed before merge.

[ask-bonk]

Concurrency bug: isInsideUseCacheProbe is a global boolean, not a counter or async-scoped flag. If two "use cache" functions deadlock concurrently, two probes will run. The first probe to finish sets setInsideUseCacheProbe(false) in finally, re-enabling probe scheduling for the second probe's inner registerCachedFunction calls — which haven't finished yet and are still inside a probe context.

The window is small but real: probe A finishes → sets flag to false → probe B's inner registerCachedFunction call reads isInsideUseCacheProbe() as false → schedules a nested probe → recursive ModuleRunner creation.

This should be a refcount instead of a boolean:

Suggested change

	setInsideUseCacheProbe(true);
	_insideProbeRefCount++;

With a corresponding _insideProbeRefCount-- in finally, and isInsideUseCacheProbe() returning _insideProbeRefCount > 0.

Alternatively, since this runs in Node.js, an AsyncLocalStorage<boolean> scoped to each probe invocation would be even more robust and avoid any shared-state concerns.

[ask-bonk]

Minor timer leak inside the probe. When runWithProbeRequestStore resolves successfully, the Promise.race completes but the timeout setTimeout on line 121 is never cleared — it's only .unref()'d. The timer callback will fire after remaining ms and create a UseCacheTimeoutError that rejects into a detached promise (the catch on line 130 won't see it since execution already left the try block via the return true on line 129).

The .unref() prevents it from keeping the process alive, so this is not a blocker, but it's wasteful and will produce an unhandled rejection in some Node versions. Store the timer ID and clear it in the finally block:

Suggested change

	]);
	let probeTimeoutTimer: ReturnType<typeof setTimeout> | undefined;
	await Promise.race([
	runWithProbeRequestStore(runner, request, async () => wrapped(...args)),
	new Promise<never>((_, reject) => {
	probeTimeoutTimer = setTimeout(() => reject(new UseCacheTimeoutError()), remaining);
	if (typeof (probeTimeoutTimer as NodeJS.Timeout).unref === "function") {
	(probeTimeoutTimer as NodeJS.Timeout).unref();
	}
	}),
	]);

Then in the finally block:

if (probeTimeoutTimer !== undefined) clearTimeout(probeTimeoutTimer);

[ask-bonk]

Production bundle size concern. UseCacheTimeoutError, UseCacheDeadlockError, getUseCacheProbe, and isInsideUseCacheProbe are unconditionally imported at the top of cache-runtime.ts. These are only used inside the if (isDev) branch (line 407), but since isDev is a runtime check (process.env.NODE_ENV === "development"), bundlers cannot tree-shake these imports — they'll be included in production builds.

The PR description says "tree-shakes out of production builds" but that's only true if the bundler can statically determine the isDev branch is dead. Vite's production build does replace process.env.NODE_ENV with "production" in some contexts, but this depends on the build pipeline and environment.

Consider either:

1. Using a dynamic import() inside the isDev branch so the modules are only loaded at runtime in dev, or
2. Verifying that vinext's production build pipeline actually eliminates the dead isDev branch (and document this assumption)

[ask-bonk]

See probe-pool.ts comment about concurrency. setInsideUseCacheProbe sets a single boolean on globalThis, which is shared across all concurrent probe invocations. If two probes overlap, the first to finish will clear the flag while the second is still running.

This should be a refcount:

Suggested change

	export function setInsideUseCacheProbe(value: boolean): void {
	export function setInsideUseCacheProbe(value: boolean): void {
	const current = ((globalThis as Record<symbol, unknown>)[INSIDE_PROBE_SYMBOL] as number) || 0;
	(globalThis as Record<symbol, unknown>)[INSIDE_PROBE_SYMBOL] = value ? current + 1 : Math.max(0, current - 1);
	}

And isInsideUseCacheProbe returns count > 0:

Suggested change

	export function setInsideUseCacheProbe(value: boolean): void {
	export function isInsideUseCacheProbe(): boolean {
	return (((globalThis as Record<symbol, unknown>)[INSIDE_PROBE_SYMBOL] as number) || 0) > 0;
	}

[ask-bonk]

Edge case: function IDs containing colons in the module path. id.lastIndexOf(":") works for most Unix paths, but on Windows, absolute paths contain colons (e.g., C:\foo\bar.ts:myExport). lastIndexOf(":") would split at the right colon here, which is correct. But if the export name itself contains a colon (unlikely but not impossible with computed exports), this would split incorrectly.

This is fine for now — just flagging it as a known limitation. A comment noting the assumption ("export names don't contain colons") would be helpful.

[ask-bonk]

Test doesn't restore NODE_ENV on failure. If the assertion on line 2970 or 2971 throws, process.env.NODE_ENV and the probe global are never restored, potentially poisoning subsequent tests.

Use a try/finally or afterEach cleanup:

Suggested change

	process.env.NODE_ENV = originalNodeEnv;
	process.env.NODE_ENV = originalNodeEnv;
	setUseCacheProbe(undefined);
	setInsideUseCacheProbe(false);

Better yet, wrap the cleanup in finally:

try {
  expect(result).toBe("result");
  expect(probeCalled).toBe(false);
} finally {
  process.env.NODE_ENV = originalNodeEnv;
  setUseCacheProbe(undefined);
  setInsideUseCacheProbe(false);
}

[ask-bonk]

Review submitted. Here's a summary of the findings:

1 issue requiring changes:

• Concurrency bug with isInsideUseCacheProbe global boolean — Two overlapping probes will race on a single boolean flag. The first probe to finish clears the flag while the second is still running, re-enabling nested probe scheduling. This should be a refcount or use AsyncLocalStorage.

4 moderate/minor issues:

• Timer leak inside the probe's Promise.race — The timeout timer in use-cache-probe-pool.ts:121 is .unref()'d but never cleared when the probe succeeds, creating a detached rejection.
• Production bundle size — use-cache-errors.ts and use-cache-probe-globals.ts are unconditionally imported at the top of cache-runtime.ts. Since isDev is a runtime check, bundlers may not tree-shake these.
• Colon-based ID splitting edge case — id.lastIndexOf(":") works but has an undocumented assumption that export names don't contain colons.
• Test cleanup not exception-safe — NODE_ENV and global probe state restoration in the test isn't wrapped in try/finally, risking test pollution on assertion failure.

github run

[james-elicx]

/bigbonk review

[ask-bonk]

Review: use cache deadlock probe (v4)

The PR has improved significantly through the review rounds. The architecture is right: fresh ModuleRunner per probe with isolated EvaluatedModules, runner.import() for ALS modules (not native import()), refcount-based recursion guard, timer cleanup in .finally(), and HMR teardown+reinit. Previous critical issues (dead scheduler file, native import(), round-robin reuse, permanent HMR disable) are all fixed.

One issue still needs a fix, and a few things are worth flagging.

Requires changes

1. Probe catch-all swallows the UseCacheTimeoutError it created. In use-cache-probe-pool.ts:131, the catch block catches all errors and returns false. But the Promise.race at line 120-128 can reject with UseCacheTimeoutError (from the timeout timer at line 123). That rejection is caught at line 131, and the probe returns false ("inconclusive"). This means the probe timeout is silently treated as "probe failed" rather than "probe also timed out", which is the correct semantic (a timed-out probe is genuinely inconclusive). However, if the wrapped function throws a real error (not a timeout), the same catch swallows it — the probe returns false when it should arguably return true (the function completed, just with an error, which is still evidence it's not deadlocked). A function that throws is not stuck. Consider: if the caught error is not a UseCacheTimeoutError, the function ran to completion (albeit with a failure), which is evidence of a deadlock in the main fill. Return true in that case.

Moderate

2. requestSnapshot.rootParams roundtrips through JSON but contains unknown values. The snapshot captures requestCtx.rootParams ?? {} (typed Record<string, unknown>) and passes it to the probe. The probe reconstructs it via createRequestContext({ rootParams: ... as Record<string, string | string[]> }). If rootParams contains non-string values at runtime, the as cast silently drops the type safety. This works today because route params are always strings, but the type mismatch could mask future bugs.

3. No integration test for the core deadlock-detection path. The unit tests cover error classes and the recursion guard (isInsideUseCacheProbe skips probe scheduling). The actual path — probe completes → reject(deadlockError) → Promise.race surfaces UseCacheDeadlockError — has zero test coverage. The fixture pages were removed due to CI timeouts, which is understandable, but a focused unit test that mocks the probe function to return true after the 10s timer fires (using fake timers) would cover this without real timeouts.

[ask-bonk]

A function that throws is not deadlocked. The catch here handles two distinct failure modes identically:

1. The probe's internal timeout fires (UseCacheTimeoutError) — genuinely inconclusive, false is correct.
2. The wrapped function throws an error — the function actually ran and completed (with a failure). If the main fill is still stuck but the probe's copy threw, that's evidence the main fill is deadlocked on shared state, not that the function itself can't run.

Consider distinguishing these:

Suggested change

	return false;
	} catch (err) {
	// If the probe timed out, the result is inconclusive.
	if (err instanceof UseCacheTimeoutError) {
	return false;
	}
	// The function threw — it ran to completion (with an error).
	// If the main fill is still stuck, this is evidence of a deadlock.
	return true;

[ask-bonk]

getRequestContext() never returns null — it creates a default context. If registerCachedFunction is somehow called outside a request scope (e.g., during module evaluation), getRequestContext() returns a fresh context with all-null fields. The snapshot will then have headers: [], urlPathname: "/", etc. — the probe will run with an empty request context.

This is probably fine for deadlock detection (the function body matters more than headers), but worth noting that navCtx?.pathname on line 429 will always be null here because serverContext defaults to null, so urlPathname will always be "/". If this is intentional, a comment would help.

[ask-bonk]

Dynamic import() inside the hot path helps with tree-shaking but adds per-call overhead in dev. Every invocation of a "use cache" function in dev mode does two dynamic imports (use-cache-errors.js and use-cache-probe-globals.js). After the first call these are cached by the module system so the cost is minimal, but it's worth noting that bundlers do cache dynamic import results — the tree-shaking benefit is real.

One alternative: hoist the imports to module scope behind a lazy singleton pattern:

let _probeModules: { UseCacheTimeoutError: ..., getUseCacheProbe: ... } | undefined;
async function getProbeModules() {
  return _probeModules ??= { ...(await import('./use-cache-errors.js')), ...(await import('./use-cache-probe-globals.js')) };
}

Not blocking — the current approach is correct.

[ask-bonk]

Good: ModuleRunner on line 22 is import type, which avoids pulling the module at runtime. The DevEnvironment import on line 20 is also import type. Both are correct.

[ask-bonk]

The teardown+reinit pattern looks correct now. tearDownUseCacheProbePool() nulls _probeEnvironment and clears the probe, then initUseCacheProbePool() sees _probeEnvironment === null, re-assigns it, and re-installs the probe closure.

One thing to watch: if invalidateAppRoutingModules() is called frequently during rapid saves, each call creates a new probe closure. The old closure is garbage-collected since setUseCacheProbe() overwrites the globalThis symbol. Any in-flight probe from the old closure will still hold a reference to _probeEnvironment via the closure capture (the _probeEnvironment! on line 47 of use-cache-probe-pool.ts). Since _probeEnvironment is reassigned to null then immediately back to the same rscEnv, this is fine — but if the environment could change, the captured null would be a problem.

[ask-bonk]

Good: the refcount approach fixes the concurrency bug from the prior review. setInsideUseCacheProbe(true) increments, setInsideUseCacheProbe(false) decrements, and isInsideUseCacheProbe() checks > 0. This correctly handles overlapping probes.

Minor nit: the function name setInsideUseCacheProbe(value: boolean) with refcount semantics is a bit misleading — enterUseCacheProbe() / leaveUseCacheProbe() would be clearer, but not worth changing now.

[ask-bonk]

Race condition with _probeEnvironment! capture. The probe closure captures _probeEnvironment by reference from the module scope. If tearDownUseCacheProbePool() is called (setting _probeEnvironment = null) while a probe is in-flight, this line reads null and the ! assertion crashes.

The window is narrow (HMR invalidation while a probe is running), but it exists. Consider capturing the environment at closure creation time:

Suggested change

	const runner = createDirectRunner(_probeEnvironment!);
	const env = _probeEnvironment!;
	const runner = createDirectRunner(env);

Or even better, capture it in initUseCacheProbePool and close over the local:

const env = environment;
setUseCacheProbe(async (msg) => {
  const runner = createDirectRunner(env);
  ...
});

[ask-bonk]

The probe timer fires even if execution completes normally, but the reject is harmless. When executionPromise wins the Promise.race at line 472, the .finally() at line 472-475 clears both timers. Since clearTimeout(probeTimer) runs synchronously after the race resolves, the 10s timer is cleared before it fires.

This is correct — the prior review's timer-leak concern has been addressed. The .finally() cleanup is the right pattern here.

[ask-bonk]

Review submitted. Here's the summary:

1 issue requiring changes:

• Probe catch-all swallows thrown errors as "inconclusive" — The catch block in use-cache-probe-pool.ts:131 returns false for all errors, but a function that throws (as opposed to timing out) actually completed — it's not stuck. If the caught error is not a UseCacheTimeoutError, the probe should return true (evidence of deadlock in the main fill).

2 moderate issues:

• rootParams type mismatch: captured as Record<string, unknown>, cast to Record<string, string | string[]> in the probe — works today but masks type safety.
• No integration test for the core deadlock-detection path (probe completes → UseCacheDeadlockError surfaced via Promise.race). A fake-timer unit test could cover this without real timeouts.

Additional observations (non-blocking):

• The _probeEnvironment! assertion in the probe closure reads from module scope and can crash if HMR teardown runs mid-probe — suggested capturing the environment in a local variable at closure creation time.
• The refcount-based isInsideUseCacheProbe correctly handles concurrent probes — good fix from prior round.
• Timer cleanup in .finally() is correct and addresses the prior timer-leak concern.
• Dynamic imports inside the dev branch are fine for tree-shaking; module cache avoids repeated overhead.

github run

[james-elicx]

/bigbonk review

[ask-bonk]

Review: use cache deadlock probe (v5)

The PR has come a long way through four review rounds — the major structural issues (dead scheduler file, native import() vs runner.import(), round-robin pool reuse, permanent HMR disable, recursive probing) are all fixed. The architecture is sound. Fresh ModuleRunner per probe, refcount-based recursion guard, timer cleanup in .finally(), and HMR teardown+reinit is the right approach.

There are still a few things that need attention before this can merge.

Requires changes

1. Probe probePromise never settles when probe returns false (inconclusive). This is the most impactful remaining issue. When the probe fires (after 10s) and returns false (inconclusive — it also hung in isolation), the .then() callback at line 451 does nothing (completed is false, so the if (completed) doesn't reject). But probePromise is a Promise<never> that has no resolve path — it can only settle via reject(). In the race at line 477, the probePromise then just dangles. This is fine when execution eventually completes or the timeout fires, but it means the probe result is silently swallowed and the 54s timeout becomes the only backstop. Consider: when the probe returns false, should the code log a warning ("cache fill may be stuck, probe was also inconclusive") to give the developer some signal before the 54s hard timeout?

2. initUseCacheProbePool early-return guard prevents reinit after HMR teardown (almost). tearDownUseCacheProbePool() sets _probeEnvironment = null, and then initUseCacheProbePool() checks if (_probeEnvironment) and returns early if truthy. Since teardown nulls it first, reinit will proceed — but the comment on line 35 says "Already initialized — no-op. The environment is the same for the lifetime of the dev server." This is misleading because HMR explicitly tears down and reinits. The comment should reflect the actual usage pattern to avoid future confusion.

Moderate

3. private variant in probe pool is dead code. The private cache branch in cache-runtime.ts (line 387) returns before the isDev deadlock-detection branch (line 401). Private cache functions never reach the probe scheduling code. The kind === "private" ? "private" : "" logic at line 95 of use-cache-probe-pool.ts can never produce "private" because the probe is never scheduled for private caches. Not a bug, but dead/misleading code.

4. rootParams type mismatch persists. The snapshot captures requestCtx.rootParams ?? {} which has type Record<string, unknown> (from UnifiedRequestContext), but UseCacheProbeRequestSnapshot declares it as Record<string, string | string[] | undefined>. The runWithProbeRequestStore parameter also uses Record<string, string | string[] | undefined>. This works because route params are always strings at runtime, but it's an implicit assumption. An explicit as cast at the capture site or a narrowing helper would make this intentional.

5. cookieHeader captured but never used. The snapshot captures cookieHeader: headers?.get("cookie") ?? undefined at line 433, but runWithProbeRequestStore never reads it — it reconstructs headers from the headers array (which already includes the Cookie header). Either remove cookieHeader from the snapshot type and capture, or use it.

[ask-bonk]

Inconclusive probe silently dangles. When completed is false (the probe also hung in isolation), nothing happens — probePromise has no resolve path and will never settle. The developer gets zero feedback until the 54s hard timeout fires.

This isn't a correctness bug (the timeout backstop works), but it defeats the purpose of early detection for the inconclusive case. At minimum, consider logging a dev-mode warning so the developer gets a signal before waiting 44 more seconds:

Suggested change

	() => {},
	.then(
	(completed) => {
	if (completed) reject(deadlockError);
	else if (typeof console !== "undefined") {
	console.warn(
	`[vinext] "use cache" fill for ${id} has been idle for 10s. ` +
	`Probe was also inconclusive — will hard-timeout at 54s.`,
	);
	}
	},
	() => {},
	);

[ask-bonk]

Misleading comment. This says "Already initialized — no-op" and "the environment is the same for the lifetime of the dev server", but invalidateAppRoutingModules() explicitly calls tearDownUseCacheProbePool() then initUseCacheProbePool() on every file change. The guard works correctly (teardown nulls _probeEnvironment, so reinit proceeds), but the comment is wrong about this being a one-time init.

Suggested change

	if (_probeEnvironment) {
	// Already initialized — no-op. The environment is the same for the
	// lifetime of the dev server.
	return;
	// Guard against double-init within the same cycle (e.g., if
	// initUseCacheProbePool is called without a preceding teardown).
	return;

[ask-bonk]

Dead code. kind can never be "private" here because private cache functions (cacheVariant === "private") return at cache-runtime.ts:396 before reaching the isDev probe-scheduling block at line 401. The probe is never scheduled for private caches.

This should either be removed (simplify to const variant = "";) or documented with a comment explaining it's a safety net for future changes.

[ask-bonk]

cookieHeader is captured but never consumed. runWithProbeRequestStore reconstructs headers from the headers array (which already includes the cookie header entry). The cookieHeader field is never read. Either remove it from the snapshot or use it in the probe to set the cookie jar explicitly.

[ask-bonk]

Type mismatch with source. rootParams is declared as Record<string, string | string[] | undefined> here, but UnifiedRequestContext.rootParams is Record<string, unknown>. The values are always strings at runtime (they're route params), but the types don't match. Consider using Record<string, unknown> here for consistency, or add an explicit narrowing cast at the capture site in cache-runtime.ts.

[ask-bonk]

Minor: JSON.stringify(args) can throw. If args contains circular references or BigInts, this will throw and the entire cachedFn will reject — not just the probe scheduling. This is unlikely for typical "use cache" arguments, but wrapping in try/catch with a fallback (empty string or "[]") would be more defensive:

Suggested change

	encodedArguments: JSON.stringify(args),
	encodedArguments: (() => { try { return JSON.stringify(args); } catch { return "[]"; } })(),

Alternatively, move the serialization into a helper.

[ask-bonk]

Good: the try/finally pattern correctly restores global state even on assertion failure. This addresses the prior review concern about test pollution.

One thing to note: vi.resetModules() on line 2958 means cache-runtime.js is re-evaluated with NODE_ENV="development", but the probe-globals.js imported on line 2947 is from the original module evaluation (before resetModules). Since the globals use Symbol.for on globalThis, this still works — but it's worth being aware that the test is mixing pre-reset and post-reset module instances. A comment noting this would help future readers.

[ask-bonk]

Review submitted. Here's a summary of the findings:

Requires changes (2):

1. Inconclusive probe silently dangles — When the probe returns false (also stuck in isolation), probePromise never settles and the developer gets zero feedback until the 54s hard timeout. Suggested adding a console.warn for the inconclusive case so developers get an early signal.

2. Misleading comment on initUseCacheProbePool guard — The comment says "Already initialized — no-op" and claims it's for the lifetime of the dev server, but HMR explicitly tears down and reinits on every file change. The guard works correctly, but the comment is wrong about the usage pattern.

Moderate (3):

3. private variant in probe pool is dead code — Private cache functions return at cache-runtime.ts:396 before reaching the probe scheduling block at line 401, so kind can never be "private" in the probe.

4. rootParams type mismatch — Snapshot captures Record<string, unknown> but declares Record<string, string | string[] | undefined>. Works at runtime but masks type safety.

5. cookieHeader captured but never consumed — The snapshot captures it, but runWithProbeRequestStore reconstructs headers from the full headers array instead.

github run

[james-elicx]

/bigbonk review

[ask-bonk]

Review: use cache deadlock probe (round 6)

This has been through five review rounds and the architecture is in good shape. The core design — fresh ModuleRunner per probe with isolated EvaluatedModules, runner.import() for ALS modules, refcount-based recursion guard, timer cleanup in .finally(), HMR teardown+reinit — is correct. CI is fully green. Previous critical issues (dead scheduler file, native import(), round-robin reuse, permanent HMR disable, recursive probing, timer leaks) are all resolved.

There are still a few things worth addressing before merge. One is a real semantic issue, one is an unfinished cleanup from a prior round, and the rest are minor but would improve the code.

Requires changes

1. cookieHeader was removed from the runtime code but is still declared in the UseCacheProbeRequestSnapshot type — or rather, it was removed from the snapshot construction but the type definition doesn't include it either, so this is actually fine. Disregard. (Self-correction: I verified the type and snapshot are consistent.)

Moderate

See inline comments.

Minor / non-blocking

See inline comments.

[ask-bonk]

Nit: Date.now() vs performance.now() inconsistency. The deadline here is computed with Date.now(), but the remaining-time check in cache-runtime.ts:443 uses performance.now() (fillDeadlineAt - performance.now() - 1_000). These two clocks have different origins — Date.now() is epoch-based, performance.now() is monotonic from process start. They can't be meaningfully subtracted from each other.

The deadline on this line is only used on line 121 as deadline - Date.now(), which is self-consistent. But if someone later tries to compare this deadline with fillDeadlineAt (which is performance.now()-based), it'll be wrong. Consider using performance.now() here too for consistency with the caller:

Suggested change

	const deadline = Date.now() + timeoutMs;
	const deadline = performance.now() + timeoutMs;

Then line 121 becomes deadline - performance.now(). The monotonic clock is also more correct for timeout math (immune to wall-clock adjustments).

[ask-bonk]

If the suggestion to use performance.now() for the deadline is accepted, this line should also change:

Suggested change

	const remaining = deadline - Date.now();
	const remaining = performance.now() >= deadline ? 0 : deadline - performance.now();

Or more simply:

const remaining = deadline - performance.now();

since the <= 0 check on the next line already handles the expired case.

[ask-bonk]

executeWithContext is started after the probe and timeout promises are constructed. This means if executeWithContext completes synchronously (or in a single microtask), the probePromise and timeoutPromise rejections become orphaned. The .catch(() => {}) handlers on lines 474 and 484 prevent unhandled rejection crashes, so this is safe — but it's worth noting that the order matters: if executeWithContext were started before the promises were set up, a fast return could resolve before the race is constructed.

The current order is correct. Just flagging it as a subtlety that a future refactor should preserve.

[ask-bonk]

rootParams type assertion masks a real mismatch. getRequestContext().rootParams is Record<string, unknown> (from UnifiedRequestContext), but the snapshot type is Record<string, string | string[] | undefined>. The as cast silently drops the type safety.

This works today because route params are always strings, but the assertion hides the fact that the types don't agree. A safer approach would be to narrow at the source:

Suggested change

	const requestSnapshot = {
	headers: headers ? Array.from(headers.entries()) : [],
	urlPathname: navCtx?.pathname ?? "/",
	urlSearch: navCtx?.searchParams?.toString() ?? "",
	rootParams: (requestCtx.rootParams ?? {}) as Record<
	rootParams: Object.fromEntries(
	Object.entries(requestCtx.rootParams ?? {}).map(([k, v]) => [
	k,
	typeof v === 'string' || Array.isArray(v) ? v : undefined,
	]),
	),

Non-blocking — the cast is fine for now, but this is a latent type hole.

[ask-bonk]

Dead code path. As noted in previous reviews, kind can never be "private" because private cache functions return at cache-runtime.ts:396 before reaching the isDev probe-scheduling block. The comment on line 95-96 acknowledges this but the logic still carries the dead branch.

Consider simplifying to just const variant = ""; and removing the comment about private caches. The comment makes it sound like the code is defensively handling something that could happen, but the code path is structurally unreachable — it's not a safety net, it's dead code.

Non-blocking.

[ask-bonk]

Pre-allocating error objects is unusual and slightly wasteful. timeoutError and deadlockError are constructed eagerly on every "use cache" invocation in dev mode, but most invocations complete quickly and never use either error. These constructors capture a stack trace (Error.captureStackTrace), which has real cost.

Consider constructing them lazily — only when the timeout or deadlock actually fires:

// In the setTimeout callback:
reject(new UseCacheTimeoutError());

// In the probe .then callback:
reject(new UseCacheDeadlockError());

The downside is that the stack trace points to the timer callback instead of the registerCachedFunction call site, which is arguably less useful anyway (the call site context is in id). The upside is avoiding stack trace capture on every cache call.

Non-blocking, but worth considering for dev perf if "use cache" is called frequently.

[ask-bonk]

Dynamic imports are fine for tree-shaking but add async overhead. After the first call, the module cache makes these near-free. But every registerCachedFunction invocation does await Promise.all([import(...), import(...)]) — two microtask yields minimum.

For a hot path in dev mode, consider hoisting to a lazy singleton so the await only happens on the first call:

let _probeModules: Awaited<ReturnType<typeof loadProbeModules>> | undefined;
async function loadProbeModules() {
  const [errors, globals] = await Promise.all([
    import("./use-cache-errors.js"),
    import("./use-cache-probe-globals.js"),
  ]);
  return { ...errors, ...globals };
}

Then: const mods = _probeModules ??= await loadProbeModules();

Non-blocking.

[ask-bonk]

setInsideUseCacheProbe(true) is called before runner.import(). The import itself can fail (e.g., module not found, transform error). If it throws, the finally block calls setInsideUseCacheProbe(false), which decrements the refcount — correct.

But: the probe function was called because !isInsideUseCacheProbe() was true in cache-runtime.ts. If the import fails and the probe returns false, the caller in cache-runtime.ts does nothing (the .then callback checks if (completed)). The isInsideUseCacheProbe flag is correctly incremented/decremented in all paths — just confirming the flow is sound.

[ask-bonk]

Good test — this is the key behavioral test that was missing in earlier rounds. The fake-timer approach to test the 10s probe scheduling and deadlock error surfacing via Promise.race is exactly right. This covers the core detection path without needing real timeouts.

One concern: vi.advanceTimersByTimeAsync(10_000) advances fake timers, but the probe function installed via setUseCacheProbe(async () => true) is a microtask (resolved promise). After advanceTimersByTimeAsync fires the 10s setTimeout, the probe's .then() callback runs in the next microtick. The single await Promise.resolve() on line 3055 should be enough to flush that, but if this test ever becomes flaky, adding a second await Promise.resolve() would be the fix.

[ask-bonk]

The refcount approach is correct for concurrent probes. Good fix from the earlier review rounds.

As noted in the v4 review, the API shape (setInsideUseCacheProbe(true/false)) is slightly misleading for refcount semantics — enterUseCacheProbe() / leaveUseCacheProbe() would be clearer. But renaming is not worth a churn-only change at this point.

[ask-bonk]

Review submitted. Here's a summary of the findings:

Overall assessment: The PR is in good shape after 5 prior review rounds. All previously identified critical issues (dead scheduler file, native import() vs runner.import(), round-robin pool reuse, permanent HMR disable, recursive probing, timer leaks) have been fixed. CI is fully green. The architecture — fresh ModuleRunner per probe, refcount-based recursion guard, timer cleanup in .finally(), HMR teardown+reinit — is correct.

Key findings in this round (all moderate/minor, no blockers):

1. Date.now() vs performance.now() inconsistency — The probe pool uses Date.now() for its deadline while cache-runtime.ts uses performance.now(). These are self-consistent within each file but could cause bugs if someone later tries to compare them. Suggested using performance.now() throughout for consistency and monotonic clock correctness.

2. rootParams type assertion — The as Record<string, string | string[] | undefined> cast hides a type mismatch with UnifiedRequestContext.rootParams (Record<string, unknown>). Works at runtime but is a latent type hole.

3. Dead code in probe pool — The kind === "private" path is structurally unreachable since private caches return before the probe scheduling block.

4. Pre-allocated error objects on every dev-mode invocation — UseCacheTimeoutError and UseCacheDeadlockError are constructed eagerly (capturing stack traces) even for fast completions. Lazy construction would avoid this overhead.

5. Dynamic imports on every call — The Promise.all([import(...), import(...)]) pattern adds async overhead on every invocation. A lazy singleton would eliminate the overhead after the first call.

github run

[james-elicx]

/bigbonk review

[ask-bonk]

Review: use cache deadlock probe (round 7)

This PR has been through six review rounds and addressed all previously identified critical issues. The architecture is solid: fresh ModuleRunner per probe with isolated EvaluatedModules, runner.import() for ALS modules, refcount-based recursion guard, timer cleanup in .finally(), HMR teardown+reinit, lazy probe module loading, and console.warn for the inconclusive case. CI is green.

I have two actionable findings (one moderate, one minor) and a few non-blocking observations.

Moderate

1. _probeModules lazy singleton survives vi.resetModules() but loadProbeModules re-evaluates the modules — In cache-runtime.ts, the _probeModules variable is at module scope and uses ??= to avoid re-importing. But when vi.resetModules() runs in tests, a new cache-runtime.js module instance is created — with its own _probeModules = undefined. This is correct for test isolation, but it means the lazy singleton only caches within a single module instance. In production (single module instance), this is fine. In tests with vi.resetModules(), this is also fine because each test gets a fresh singleton. No action needed — flagging for documentation clarity only.

2. Probe pool static import in index.ts pulls vinext/shims/use-cache-probe-globals and vinext/shims/use-cache-errors eagerly at plugin load time — Line 19 of index.ts statically imports use-cache-probe-pool.js, which in turn statically imports setUseCacheProbe, setInsideUseCacheProbe from vinext/shims/use-cache-probe-globals and UseCacheTimeoutError from vinext/shims/use-cache-errors. These modules are loaded when the Vite plugin is registered, even if the dev server is never started (e.g., vite build). The functions are only called inside configureServer(), so this is harmless at runtime, but it's unnecessary module loading during production builds. Consider making the import in index.ts dynamic (inside configureServer) or lazy. Non-blocking — the cost is negligible for a plugin module.

Minor

See inline comments.

What looks good

• Fresh ModuleRunner per probe with runner.close() in finally — correctly avoids stale module state.
• Refcount-based isInsideUseCacheProbe — handles concurrent probes correctly.
• env captured in local variable at closure creation time (line 43 of use-cache-probe-pool.ts) — immune to HMR teardown mid-probe.
• Timer cleanup in .finally() at both levels (probe-pool and cache-runtime) — no timer leaks.
• Lazy singleton _probeModules — avoids per-call dynamic import overhead after the first invocation.
• try/finally in tests — properly restores global state on assertion failure.
• Error classes set this.name — stack traces show the correct class name.
• JSON.stringify wrapped in try/catch with "[]" fallback — handles circular references gracefully.
• Inconclusive probe logs console.warn — gives developers a signal before the 54s hard timeout.
• HMR teardown+reinit in invalidateAppRoutingModules — probes continue working after file changes.

Overall this is in good shape. The architecture matches the Next.js approach (vercel/next.js#93500, #93538) adapted for Vite's ModuleRunner isolation model. The inline comments are thorough and explain the non-obvious design decisions well.

[ask-bonk]

Dead code path. cacheVariant === "private" returns at cache-runtime.ts:412-421 before reaching the isDev block at line 426, so the probe is never scheduled for private caches. variant here is always "".

This has been flagged in multiple prior rounds. Consider simplifying to just const variant = ""; with a one-line comment like // Private caches return before the probe block in cache-runtime.ts.

Non-blocking, but the current code reads like it handles something that structurally cannot happen.

[ask-bonk]

Nit: performance.now() is the right choice here — consistent with the caller (cache-runtime.ts:428 uses performance.now() for fillDeadlineAt). The monotonic clock is also correct for timeout arithmetic (immune to wall-clock adjustments). Good fix from the prior round's Date.now() feedback.

[ask-bonk]

Minor: eager error construction on every dev-mode "use cache" invocation. UseCacheTimeoutError and UseCacheDeadlockError are loaded here (via the lazy singleton) but constructed later on lines 484 and 501. So the classes are loaded eagerly, but the instances are only constructed when needed — inside the setTimeout callbacks.

Wait, looking more carefully: the errors are constructed inside the setTimeout callbacks (line 484: reject(new UseCacheDeadlockError()), line 501: reject(new UseCacheTimeoutError())), not eagerly. This is already the lazy construction pattern. Good — no wasted stack trace capture on the fast path.

Disregard the prior round's concern about eager construction; that was fixed.

[ask-bonk]

Subtle: rootParams narrowing is correct but verbose. The Object.fromEntries(Object.entries(...).map(...)) on lines 456-461 narrows Record<string, unknown> to Record<string, string | string[] | undefined> by filtering out non-string, non-array values. This is the right approach (addresses the type mismatch flagged in prior rounds) — it's explicit narrowing rather than an unsafe cast.

The as cast on line 461 is now a documentation cast rather than a lie, since the .map() already guarantees the value types. Acceptable.

[ask-bonk]

Ordering note (for future maintainers): executionPromise is started after probePromise and timeoutPromise are constructed. This ordering is correct — if executeWithContext completed synchronously (or in a single microtask), Promise.race would still resolve with its value since it's first in the promises array (line 511). The .catch(() => {}) handlers on lines 497 and 507 prevent unhandled rejections from the losing promises. Sound design.

[ask-bonk]

Good test coverage for the core detection path. The fake-timer approach to test the 10s probe + deadlock error surfacing via Promise.race is exactly right. One thing to watch: vi.advanceTimersByTimeAsync(10_000) fires the 10s setTimeout, then the probe's async () => true resolves in a microtask, then .then((completed) => { if (completed) reject(...) }) fires in another microtask. The single await Promise.resolve() on line 3107 should flush that chain, but if this test ever becomes flaky, adding a second await Promise.resolve() would be the fix.

[ask-bonk]

The lastIndexOf(":") approach for splitting "<modulePath>:<exportName>" works correctly for Unix paths and even Windows paths (where C:\ has a colon before the backslash — lastIndexOf finds the export separator, not the drive letter).

The comment on line 81 documents the assumption that export names don't contain colons. Worth noting that on Windows, module IDs like C:/foo/bar.ts:myExport would also work correctly since lastIndexOf returns the rightmost colon. Good.

[ask-bonk]

Minor: static import loads probe pool at plugin registration time. initUseCacheProbePool and tearDownUseCacheProbePool are only used inside configureServer() (dev only). A dynamic import() inside configureServer would avoid loading use-cache-probe-pool.js (and its transitive imports of use-cache-probe-globals and use-cache-errors) during vite build.

Non-blocking — Vite plugins are dev-time artifacts and the module loading cost is trivial. But if bundle size of the plugin itself ever matters, this is low-hanging fruit.

[ask-bonk]

Review submitted. Here's a summary:

Overall assessment: The PR is in good shape after six prior review rounds. All critical issues from earlier rounds (dead scheduler file, native import() vs runner.import(), round-robin pool reuse, permanent HMR disable, recursive probing, timer leaks) have been resolved. CI is green. The architecture — fresh ModuleRunner per probe, refcount-based recursion guard, timer cleanup in .finally(), lazy module loading, HMR teardown+reinit — is correct and well-documented.

Findings (all non-blocking):

1. Dead code path — variant in the probe pool can never be "private" since private caches return before the probe block. Suggested simplifying to const variant = "".

2. Static import in index.ts — The probe pool is statically imported at plugin registration time, loading probe-globals and error modules even during vite build. A dynamic import inside configureServer() would be cleaner, though the cost is negligible.

3. Several inline observations confirming correctness of the rootParams narrowing, error lazy construction, performance.now() consistency, promise ordering, and test timer flushing.

No blockers remaining.

github run

[james-elicx]

/bigbonk review

[ask-bonk]

Review: use cache deadlock probe (round 8)

This PR has been through seven review rounds and the architecture is solid. The design — fresh ModuleRunner per probe with isolated EvaluatedModules, runner.import() for ALS modules, refcount-based recursion guard, timer cleanup in .finally(), HMR teardown+reinit, lazy probe module loading — is correct and well-implemented.

I have one moderate issue, two minor issues, and some non-blocking observations.

Moderate

1. invalidateAppRoutingModules is now async but its callers silently swallow errors. The server.watcher.on("add"/"unlink") handlers call invalidateAppRoutingModules().catch(() => {}). The catch(() => {}) silently swallows all errors — not just probe-pool errors, but also failures in invalidateAppRouteCache(), invalidateRscEntryModule(), and invalidateRootParamsModule() which were previously synchronous and would throw visibly. If any of those three functions throw, the developer gets no feedback and the dev server silently operates with a stale route cache. At minimum, the catch should log the error.

Minor

2. kind field passed to probe is dead code. The kind: cacheVariant at cache-runtime.ts:471 can never be "private" because private caches return at line 412-422 before reaching the isDev block. In the probe pool, variant is hardcoded to "" (line 98). The kind field in UseCacheProbe message type exists but serves no purpose — it's always "". Consider removing it from the message type and the probe function, or at minimum adding a comment that this is always "" today.

3. Probe pool initialization is fire-and-forget with silent error swallowing. At index.ts:2174, getProbePoolModule().then(...).catch(() => {}) means if the dynamic import or initUseCacheProbePool fails, the developer gets no error and deadlock detection is silently disabled for the session. A console.warn in the catch would make this diagnosable.

Non-blocking observations

• The env local capture at use-cache-probe-pool.ts:43 correctly immunizes against HMR teardown mid-probe. Good pattern.
• Error classes correctly set this.name. The isUseCacheTimeoutError/isUseCacheDeadlockError type guards use duck-typed digest checking, which is the right approach for cross-realm compatibility.
• The lazy singleton _probeModules pattern in cache-runtime.ts eliminates per-call async overhead after the first invocation — correct optimization.
• The try/finally pattern in tests properly restores global state on assertion failure.
• Timer cleanup in .finally() at both levels (probe-pool and cache-runtime) prevents timer leaks.
• The JSON.stringify(args) try/catch with "[]" fallback handles circular references gracefully.

Overall the code is in good shape. The three items above are the only things I'd want addressed before merge.

[ask-bonk]

invalidateAppRoutingModules is now async, but .catch(() => {}) silently swallows all errors — including failures in the three synchronous invalidation calls (invalidateAppRouteCache, invalidateRscEntryModule, invalidateRootParamsModule) that were previously uncaught-and-visible. If any of those throw, the dev server silently operates with a stale route cache and the developer gets no feedback.

Suggested change

	invalidateAppRoutingModules().catch(() => {});
	invalidateAppRoutingModules().catch((err) => {
	console.warn("[vinext] Failed to invalidate app routing modules:", err);
	});

Same for the "unlink" handler on line 2103.

[ask-bonk]

Same concern as the "add" handler — silently swallowing all errors.

Suggested change

	invalidateAppRoutingModules().catch(() => {});
	invalidateAppRoutingModules().catch((err) => {
	console.warn("[vinext] Failed to invalidate app routing modules:", err);
	});

[ask-bonk]

If the probe pool fails to initialize (import error, environment issue, etc.), deadlock detection is silently disabled for the entire dev session with no diagnostic output.

Suggested change

	.catch(() => {});
	getProbePoolModule()
	.then(({ initUseCacheProbePool }) => initUseCacheProbePool(rscEnv))
	.catch((err) => {
	console.warn("[vinext] Failed to initialize use-cache probe pool:", err);
	});

[ask-bonk]

Nit: kind in this message type is structurally always "" — private caches return before the probe scheduling block in cache-runtime.ts (line 412-422), and the probe pool hardcodes variant = "" (line 98). Consider either removing kind from the type or adding a doc comment noting it's always empty string today. This avoids giving future readers the impression that the probe needs to handle different cache variants.

[ask-bonk]

cacheVariant here is never "private" (private caches return at line 412-422 before reaching this block), and the probe pool ignores this field entirely (it hardcodes variant = ""). This is dead data — it's computed, serialized, and sent but never read.

Non-blocking, but removing it would simplify the message and avoid confusion.

[ask-bonk]

Minor: setInsideUseCacheProbe(true) is called before the runner.import() calls on lines 62 and 87, which can fail (module not found, transform errors, etc.). If they throw, setInsideUseCacheProbe(false) in finally correctly decrements the refcount — so this is safe. Just confirming the flow is sound for future readers.

[ask-bonk]

Review submitted. Here's a summary of the findings:

1 moderate issue:

• invalidateAppRoutingModules was made async to support probe pool teardown/reinit, but .catch(() => {}) now silently swallows errors from the three synchronous invalidation functions that were previously uncaught-and-visible. If invalidateAppRouteCache() or invalidateRscEntryModule() throws, the dev server silently operates with stale routes and the developer gets no feedback. Should log the error at minimum.

2 minor issues:

• kind field in the probe message is dead code — private caches return before the probe scheduling block, and the probe pool hardcodes variant = "". The field is serialized and sent but never read.
• Probe pool initialization at index.ts:2174 is fire-and-forget with .catch(() => {}), meaning a failed init silently disables deadlock detection for the entire session with no diagnostic output.

Non-blocking observations (all positive):

• Architecture is sound after 7 prior rounds of review
• Fresh ModuleRunner per probe, local env capture immune to HMR teardown, refcount-based recursion guard, timer cleanup in .finally(), lazy singleton for probe modules, try/finally in tests — all correct
• Error classes properly set this.name and use duck-typed digest checking for cross-realm compatibility

github run