fix(cache-runtime): attach inner "use cache" call site as cause of nested-dynamic error

[Divkix]

Description

Ports Next.js PR #93707 to vinext.

When a nested "use cache" propagates a dynamic cache life (revalidate: 0 or expire < 5 minutes) to a parent without an explicit cacheLife(), the error now attaches the inner invocation as cause. This gives developers both stacks: the outer that threw, and the inner that propagated the dynamic life.

Changes

• cache-runtime.ts: Add NestedDynamicUseCacheError class (captured eagerly at the inner call site while still on the synchronous stack). Track hasExplicitRevalidate / hasExplicitExpire on CacheContext. Propagate dynamicNestedCacheError via ??= (first dynamic child wins). Throw with cause when the outer cache lacks explicit cacheLife for the relevant field. Gate the throw on VINEXT_PRERENDER === "1" or NODE_ENV === "development", matching Next.js (which only throws when the work unit type is prerender or request in dev).
• cache.ts: cacheLife() now sets hasExplicitRevalidate / hasExplicitExpire flags on the current CacheContext. Update CacheContextLike type to match.
• shims.test.ts: New unit tests covering revalidate: 0, short expire, explicit outer cacheLife, first-child-wins semantics, production no-throw gate, dev throw gate, and a minimum-wins documenting test.

Behavioral change: lifeConfigs propagation to parent

Beyond the error-cause feature, this PR adds parentCtx.lifeConfigs.push(effectiveLife) to propagate the inner cache's resolved life into the outer's lifeConfigs. This fills a gap that Next.js handles via propagateCacheLifeAndTagsToRevalidateStore and is load-bearing for the new error detection (without it, the threshold checks could never fire).

This changes caching behavior in production, not just during prerender/dev. Pre-PR, a nested cacheLife({ revalidate: 0 }) inner did not affect the outer's effective revalidate at all — the outer used its own lifeConfigs only. Post-PR, the outer's effective revalidate is 0 via minimum-wins (Math.min(60, 0) === 0) even if the outer called cacheLife({ revalidate: 60 }). The outer silently stops being cached in this scenario.

This matches Next.js's propagateCacheLifeAndTagsToRevalidateStore semantics. The minimum-wins documenting test in shims.test.ts ("explicit cacheLife on outer suppresses throw but minimum-wins still applies to effective cache life") explicitly verifies and pins this behavior.

Error message cleanup

Both nested-dynamic error messages now consistently backtick "use cache" (matching Next.js). Messages are also context-aware: they say "in development" when NODE_ENV=development (without VINEXT_PRERENDER) so dev-mode users aren't told they're prerendering. VINEXT_PRERENDER takes priority over NODE_ENV when both are set.

Related Issue

Closes #1194

Potential Risk & Impact

• The eager Error allocation on every nested public cache call is required for correct stack capture but adds minor overhead. Private cache parents are intentionally excluded. Matches Next.js; lazy capture is a possible future optimization if profiling ever shows this is hot.
• The lifeConfigs propagation described above changes minimum-wins behavior for nested caches in production. This is intentional, matches Next.js, and is explicitly tested.

How Has This Been Tested?

• pnpm test tests/shims.test.ts — 918/918 pass (including new nested-dynamic, dev/prod gate, and minimum-wins tests)
• pnpm run check — format, lint, and type checks clean across all 587 files
• Cache-adjacent suites (isr-cache, cache-control, cache-for-request, fetch-cache, app-page-cache) all pass

[pkg-pr-new]

Open in StackBlitz

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

commit: a48ca17

[james-elicx]

/bigbonk review

[ask-bonk]

Good port of Next.js PR #93707. The core logic — eagerly capturing NestedDynamicUseCacheError at the inner call site, propagating it to the parent context via ??=, and throwing with cause when the outer lacks explicit cacheLife — is correct and matches the upstream semantics. Tests are solid and well-structured.

A few items worth discussing:

[ask-bonk]

Error.captureStackTrace is V8-specific (Node.js, Cloudflare Workers). If vinext ever targets a non-V8 runtime (e.g. JavaScriptCore in Bun), this will throw. The upstream Next.js PR has the same limitation since they only target V8 runtimes.

A defensive guard would be cheap:

Suggested change

	Error.captureStackTrace(eagerError, runCachedFunctionWithContext);
	if (typeof Error.captureStackTrace === 'function') {
	Error.captureStackTrace(eagerError, runCachedFunctionWithContext);
	}

Not blocking — just something to consider for robustness, since the super() call in the constructor already captures a stack (just with the constructor frame included).

[ask-bonk]

Subtle: cacheLife({ expire: 60 }) reaches here with resolvedConfig = { ...cacheLifeProfiles.default, ...{ expire: 60 } }, which means resolvedConfig.revalidate is 900 (from the default profile). This sets hasExplicitRevalidate = true even though the user only specified expire.

In practice this works correctly — if you call cacheLife() at all, you've made a conscious choice, so suppressing the nested-dynamic error is appropriate. But the flag name hasExplicitRevalidate is slightly misleading since it really means "cacheLife() was called and the resolved config includes revalidate" rather than "the user explicitly passed a revalidate value".

This matches the upstream Next.js behavior (their hasExplicitRevalidate is set on the work unit store level, not per-field), so not a bug — just worth a clarifying comment for future maintainers.

[ask-bonk]

This propagation of effectiveLife into the parent's lifeConfigs is new behavior beyond what the upstream PR #93707 adds. In Next.js, the inner cache's life is propagated to the parent through a different mechanism (propagateCacheEntryMetadata), not by pushing into lifeConfigs directly.

The PR description mentions this: "Default cache life profiles are propagated upward via parentCtx.lifeConfigs.push(effectiveLife), which may slightly alter minimum-wins behavior for deeply nested caches."

Is this intentional? It means the outer's resolveCacheLife() will include the inner's effective life in its minimum-wins computation, which could change the outer's effective revalidate/expire even when the outer has its own explicit cacheLife(). For example:

• Outer calls cacheLife({ revalidate: 300 })
• Inner resolves { revalidate: 60 }
• The outer's lifeConfigs now has both; minimum-wins gives revalidate: 60

This seems like it might be the intended minimum-wins behavior for nested caches, but it's a separate concern from the error-cause feature and could cause subtle behavioral changes. Consider splitting this into a separate PR if it's intentional new behavior, or removing it if it's just scaffolding for the error propagation.

[ask-bonk]

The comment says "We can't easily test exact stack content in unit tests" — but you could verify that the cause is specifically from innerA (not innerB) by checking the cause's identity or message against a known marker. For example, you could verify thrown.message contains "revalidate" (from innerA's revalidate: 0) rather than "expire" (from innerB's expire: 120), which would actually test the first-child-wins semantics more rigorously:

Suggested change

	expect(thrown!.cause).toBeDefined();
	// The cause should be from the FIRST dynamic child (innerA), verified
	// by the outer error message being about revalidate (not expire).
	expect(thrown!.message).toContain('revalidate');

[ask-bonk]

There's a subtle interaction here: both the revalidate === 0 and expire < DYNAMIC_EXPIRE checks run sequentially. If the effective life has both revalidate === 0 AND expire < DYNAMIC_EXPIRE, only the revalidate error is thrown (the expire check is unreachable). This matches Next.js behavior where revalidate: 0 takes precedence.

However, there's an edge case: if dynamicNestedCacheError is set but the effective life is neither revalidate === 0 nor expire < DYNAMIC_EXPIRE (because the outer's own cacheLife() pushed the values above the thresholds), the error is silently discarded. This is correct — the outer made an explicit choice that overrides the dynamic child — but it only works because lifeConfigs.push(effectiveLife) at line 621 ensures the outer's minimum-wins includes the inner's values. So lines 620-622 and 637-652 are tightly coupled — good to add a comment noting this dependency.

[ask-bonk]

Review posted successfully. Here's a summary of the feedback:

Overall: Good port of Next.js PR #93707. The core error propagation logic is correct and well-tested.

5 comments posted:

1. Error.captureStackTrace (cache-runtime.ts:601) — V8-specific API. Suggested a defensive guard for non-V8 runtime compatibility, since the constructor already captures a stack.

2. hasExplicitRevalidate/hasExplicitExpire flags (cache.ts:773) — These are always true when cacheLife() is called because resolvedConfig inherits from the default profile. Not a bug (matches Next.js behavior), but the flag name is slightly misleading — suggested a clarifying comment.

3. parentCtx.lifeConfigs.push(effectiveLife) (cache-runtime.ts:620) — This is new behavior beyond what the upstream PR adds. Propagating the inner's effective life into the parent's lifeConfigs changes the minimum-wins computation for the outer cache. Called out that this is a separate behavioral change from the error-cause feature and may warrant its own PR.

4. First-child-wins test (shims.test.ts:4316) — The test could more rigorously verify first-child-wins semantics by asserting on the outer error message content (revalidate vs expire).

5. Tight coupling between lines 620-622 and 637-652 (cache-runtime.ts:638) — The error detection depends on the lifeConfigs propagation to make the effective life reflect the inner's dynamic values. Suggested documenting this dependency.

github run

[james-elicx]

/bigbonk review

[ask-bonk]

Second review pass on the nested-dynamic cache error feature. The previous review's comments were well-addressed in the updated code (the Error.captureStackTrace guard, the clarifying comment on hasExplicitRevalidate, and the comments documenting the coupling between lifeConfigs.push and the error detection). The implementation is solid.

I have two remaining concerns — one is a real behavioral issue, one is a test gap. Neither is blocking, but the first one should be considered before merge.

[ask-bonk]

Behavioral concern with lifeConfigs.push and executeWithContext callers.

The private variant (line 441) and dev mode (line 449) both call executeWithContext, which calls runCachedFunctionWithContext. This means:

1. A "use cache: private" inner will eagerly allocate a NestedDynamicUseCacheError when nested inside a public parent, and will push its effectiveLife into the parent's lifeConfigs. This is correct — private caches are excluded from being parents that throw (line 605 checks parentCtx.variant !== "private"), but they can still propagate dynamic life up to a public parent.

2. However, executeWithContext also calls recordRequestScopedCacheLife(effectiveLife) (line 564), which writes the inner's resolved life to the request-scoped store. This happens before the outer runCachedFunctionWithContext resolves its own effective life and stores it. The request-scoped cache life uses minimum-wins accumulation (_setRequestScopedCacheLife), so order doesn't matter for correctness — but it's worth noting this is a side effect of the refactor.

Not blocking, but good to verify that the request-scoped cache life accumulation order hasn't changed relative to main for the private and dev-mode paths.

[ask-bonk]

Prerendering guard is missing.

Both error messages say "which is not allowed during prerendering", but the throw is unconditional — it fires during normal dynamic SSR too, not just prerendering. In Next.js, the equivalent check is gated by the work unit type being a prerender (workUnitStore.type === 'prerender'). This means a normal dynamic request with nested "use cache" where the inner has revalidate: 0 will throw an error claiming it's "not allowed during prerendering" even though no prerendering is happening.

This is the most significant issue in the PR. Options:

1. Gate the throw on being inside a prerender work unit (check workUnitAsyncStorage)
2. Remove "during prerendering" from the error message to make it accurate for all contexts
3. Accept the divergence from Next.js and document it

I'd suggest option 1 for parity, but if the work unit store isn't reliably available in all vinext contexts, option 2 is a reasonable alternative.

[ask-bonk]

Minor: the eager Error allocation happens on every nested public cache call, including cache hits in the shared cache path. Looking at the call flow:

• registerCachedFunction → shared cache HIT → returns early (line 465), never calls runCachedFunctionWithContext
• registerCachedFunction → shared cache MISS → calls runCachedFunctionWithContext → allocates error
• executeWithContext → always calls runCachedFunctionWithContext → allocates error

So for the shared cache path, the allocation only happens on miss, which is fine. For private and dev-mode paths via executeWithContext, it happens on every invocation (since those skip cache lookup). In practice, the overhead is negligible — Error construction is cheap and the object is short-lived if not dynamic. Just noting for completeness.

[ask-bonk]

Test gap: verify the return value semantics when outer has explicit cacheLife.

This test verifies the error is suppressed, but it would be more thorough to also verify the effective cache life that the outer resolves to. With cacheLife({ revalidate: 60 }) on the outer and cacheLife({ revalidate: 0 }) on the inner, the outer's effectiveLife resolves to revalidate: 0 via minimum-wins (because the inner's resolved life is pushed into parentCtx.lifeConfigs). This means the outer's result won't actually be cached by MemoryCacheHandler (it returns early for revalidate: 0).

This is arguably correct (minimum-wins semantics), but it might surprise users who expect cacheLife({ revalidate: 60 }) on the outer to override the inner's dynamic value. A test that calls outerCached() twice and verifies the function executes both times (no caching) would document this behavior explicitly.

[ask-bonk]

Review posted successfully. Here's a summary of the feedback:

Overall: The implementation is solid. The previous review's comments were well-addressed (captureStackTrace guard, clarifying comment on flag names, coupling documentation). The core error propagation logic is correct.

4 comments posted:

1. lifeConfigs.push and executeWithContext callers (cache-runtime.ts:637) — Noted that the private variant and dev-mode paths via executeWithContext also flow through runCachedFunctionWithContext, which means they propagate life configs up and call recordRequestScopedCacheLife. Not a bug, but worth verifying the request-scoped cache life accumulation order hasn't changed vs main.

2. Missing prerendering guard (cache-runtime.ts:670) — The most significant issue. Both error messages say "not allowed during prerendering" but the throw is unconditional — it fires during normal dynamic SSR too. In Next.js, the equivalent check is gated by the work unit type being a prerender. Suggested either gating on the prerender work unit or removing "during prerendering" from the message.

3. Eager Error allocation (cache-runtime.ts:606) — Minor note for completeness. The allocation only happens on cache misses for the shared path (good), but happens on every invocation for private/dev-mode paths. Overhead is negligible.

4. Test gap for explicit cacheLife override semantics (shims.test.ts:4259) — The "does not throw" test could be more thorough by verifying that the outer's result actually isn't cached (since minimum-wins resolves to revalidate: 0 despite the outer's explicit revalidate: 60), documenting this potentially surprising behavior.

github run

[james-elicx]

/bigbonk review

[ask-bonk]

Good port of Next.js PR #93707. The three prior review rounds covered the major points well and the code has been iterated thoughtfully. The core logic — eager error capture, ??= first-child-wins propagation, prerender/dev gating, and hasExplicit* suppression — is correct and well-documented.

Two remaining items I'd want addressed before merge, one nit, and one observation.

[ask-bonk]

Dev-mode error message is misleading.

The shouldThrow gate correctly fires in both prerender (VINEXT_PRERENDER=1) and development (NODE_ENV=development). But both error messages (lines 68-73, 75-81) say "which is not allowed during prerendering". During dev SSR, no prerendering is happening — the message is confusing.

Next.js gets away with this because their error messages are generic and the throw location is in shared render code. Here, it's worth either:

1. Making the message context-aware:

Suggested change

	(process.env.VINEXT_PRERENDER === "1" || process.env.NODE_ENV === "development");
	const shouldThrow =
	typeof process !== "undefined" &&
	(process.env.VINEXT_PRERENDER === "1" || process.env.NODE_ENV === "development");
	const duringPhrase = process.env.VINEXT_PRERENDER === "1" ? "during prerendering" : "in development";

...and interpolating duringPhrase into the error messages, or

2. Using a neutral phrase like "which is not allowed" (drop "during prerendering") in both messages.

Option 2 is simpler and always accurate. The linked docs page already explains the full context.

[ask-bonk]

Timing of resolveCacheLife vs the throw checks is correct but fragile.

The flow is: inner completes → pushes into parentCtx.lifeConfigs (line 655) → outer's fn body finishes → outer calls resolveCacheLife(ctx.lifeConfigs) (line 642) which now contains both the outer's own cacheLife() calls and the inner's pushed effectiveLife → throw checks at lines 703-716 use this merged result.

This works because cacheContextStorage.run(ctx, () => fn(...args)) creates a new context for the inner, but the inner's post-execution code pushes into the parent context's lifeConfigs. The parent's resolveCacheLife call happens after fn(...args) returns, so sequencing is guaranteed.

However, the correctness depends on the inner's await runCachedFunctionWithContext(...) completing (including its parentCtx.lifeConfigs.push) before the outer's resolveCacheLife runs. This is guaranteed by await semantics today, but if anyone ever refactored the inner to use fire-and-forget or Promise.all with short-circuit, the ordering could break silently. Consider adding a brief comment at line 642 noting this dependency:

// Note: must run after fn(...args) returns — the inner's
// runCachedFunctionWithContext has already pushed its effectiveLife
// into ctx.lifeConfigs by this point (via the parentCtx propagation).

[ask-bonk]

Nit: this test may not actually test the production gate.

Vitest defaults NODE_ENV to "test", not "development". The test sets NODE_ENV = "production" and deletes VINEXT_PRERENDER to verify the throw doesn't fire. But "test" would also not fire (since the gate checks === "development"), so this test would pass even if the gate were broken to check !== "production" instead of === "development".

Consider adding a sibling assertion that explicitly sets NODE_ENV = "development" (without VINEXT_PRERENDER) and confirms the throw does fire. That would fully verify both sides of the gate.

[ask-bonk]

Minor observation: the eager NestedDynamicUseCacheError allocation happens for every nested public cache call, including those where the inner resolves a non-dynamic cache life (e.g., cacheLife('days')). The error is then silently discarded at lines 662-668 when the condition doesn't match.

For a hot path where most nested caches are not dynamic, this is a wasted allocation + stack capture per call. In Next.js this is acceptable because the same pattern exists. But if profiling ever shows this as a bottleneck, a lazy approach (capture the error after resolving the effective life, only if dynamic) would eliminate the waste — at the cost of the stack pointing to the post-execution site rather than the original call site.

Not blocking — just noting for future optimization if cache-heavy workloads show up in profiles.

[ask-bonk]

Review posted successfully. Here's a summary of the feedback:

Overall: Good port of Next.js PR #93707. The core logic is correct and well-documented after three prior review rounds.

4 comments posted:

1. Dev-mode error message is misleading (cache-runtime.ts:701) — The shouldThrow gate correctly fires in both prerender and development, but both error messages say "not allowed during prerendering". During dev SSR, no prerendering is happening. Suggested either making the message context-aware or using a neutral phrase like "which is not allowed" (simpler, always accurate).

2. resolveCacheLife timing is correct but fragile (cache-runtime.ts:639-642) — The sequencing works because await guarantees the inner's lifeConfigs.push completes before the outer's resolveCacheLife runs. But this invariant could break silently in a future refactor. Suggested adding a brief comment documenting this dependency.

3. Production gate test gap (shims.test.ts:4277) — The "does not throw outside prerender/dev" test sets NODE_ENV=production, but since Vitest defaults to NODE_ENV=test (which also wouldn't trigger the throw), the test would pass even with a broken gate. Suggested adding a sibling test that sets NODE_ENV=development and confirms the throw does fire.

4. Eager error allocation overhead (cache-runtime.ts:624) — The NestedDynamicUseCacheError is allocated for every nested public cache call, even when the inner resolves a non-dynamic life. Not blocking — matches Next.js behavior — but noted as a future optimization target if cache-heavy workloads show up in profiles.

github run

[james-elicx]

/bigbonk review

[ask-bonk]

Good port of Next.js PR #93707. The iteration across 4 commits shows thoughtful responses to prior review feedback — context-aware error messages, the prerender/dev gate, captureStackTrace guard, and extensive commenting are all welcome improvements over a naive 1:1 port.

The core logic is correct: eager error capture at the inner call site, ??= first-child-wins propagation, hasExplicit* flag suppression, and the prerender/dev-only throw gate all match the upstream semantics. Tests are comprehensive and well-structured, including the dev-mode companion test that pins both sides of the gate.

One real issue (medium severity), one structural divergence worth documenting, and a couple of nits follow.

[ask-bonk]

The effectiveLife.revalidate === 0 check is subtly different from the upstream gate.

In Next.js, the throw is gated on the rdcResult.entry.revalidate (the inner's cache entry metadata) being 0, which represents the stored revalidate value. Here, effectiveLife is the outer's resolved cache life after minimum-wins merges all lifeConfigs (including the inner's pushed effectiveLife). In practice these produce the same result for the revalidate === 0 case (since Math.min(anything, 0) === 0), but they diverge semantically:

• If the outer calls cacheLife({ revalidate: 60 }) and the inner has revalidate: 0, the outer's effectiveLife.revalidate is 0 via minimum-wins. The !ctx.hasExplicitRevalidate guard is false (outer called cacheLife), so the throw is correctly suppressed.
• If the outer calls cacheLife({ revalidate: 0 }) (explicitly dynamic) and the inner also has revalidate: 0, the outer's effectiveLife.revalidate is 0, hasExplicitRevalidate is true, throw is suppressed. Correct.

So the behavior is correct in all cases I can construct. The semantic difference (checking the merged outer effective life vs the inner's entry metadata) doesn't produce observable differences because the throw guards (hasExplicit*) cover the cases where the merge could mask the inner's contribution. Just noting this for future maintainers — the comment at line 719 does a good job explaining the coupling, but mentioning this semantic difference would help.

[ask-bonk]

The parentCtx.lifeConfigs.push(effectiveLife) propagation is the most significant behavioral change beyond the error-cause feature.

The upstream PR #93707 does not add this propagation — Next.js already had propagateCacheLifeAndTagsToRevalidateStore which propagates the inner's resolved metadata into the outer's revalidate store. The vinext equivalent didn't exist before this PR, so this push is filling that gap.

This is load-bearing for correctness (the error detection depends on it, as the comment explains), and the minimum-wins documenting test at line 4397 explicitly verifies the consequence (outer re-executes because minimum-wins resolves to revalidate: 0 despite the outer's explicit cacheLife({ revalidate: 60 })). That's well done.

However, this is a standalone behavioral change separate from the error-cause feature. Pre-PR, a nested cacheLife({ revalidate: 0 }) inner would not affect the outer's effective revalidate at all — the outer would use its own lifeConfigs only. Post-PR, the outer's effective revalidate is 0 because the inner's life is pushed into its lifeConfigs. This changes caching behavior for any existing nested "use cache" setup where the inner is dynamic and the outer is not, even in production where the throw doesn't fire.

The PR description acknowledges this ("may slightly alter minimum-wins behavior for deeply nested caches without explicit cacheLife()"), and it matches Next.js semantics. But it would be worth calling this out in the commit message as a behavioral change, not just an implementation detail of the error feature.

[ask-bonk]

Nit: the priority order here means that when VINEXT_PRERENDER=1 and NODE_ENV=development are both set (e.g., running a local prerender with dev config), the message says "in development" rather than "during prerendering". This is unlikely in practice (prerender usually implies production NODE_ENV), but if it's intentional, a comment would help. If not:

Suggested change

	function nestedCacheContextPhrase(): string {
	function nestedCacheContextPhrase(): string {
	if (typeof process === "undefined") return "during prerendering";
	if (process.env.VINEXT_PRERENDER === "1") return "during prerendering";
	if (process.env.NODE_ENV === "development") return "in development";
	return "during prerendering";
	}

This gives VINEXT_PRERENDER priority, which seems more accurate — if the prerender flag is set, the user is actually prerendering regardless of NODE_ENV.

[ask-bonk]

Good addition of both the production gate test and the dev-mode companion test (line 4319). The dev test at line 4319 was the right call from prior review — it pins both branches of the gate.

One subtle issue remains: Vitest sets NODE_ENV=test by default. If the shouldThrow gate were accidentally written as NODE_ENV !== 'production' instead of NODE_ENV === 'development', this test would still pass (because you set it to "production" to test the negative case). But the dev-mode test at line 4331 catches that regression by explicitly testing "development". So the pair together is solid.

The only remaining gap: there's no test that verifies NODE_ENV=test (the Vitest default) does not trigger the throw. Currently that's implicitly covered by any test that calls registerCachedFunction without setting env vars (since "test" !== "development" and VINEXT_PRERENDER is unset). But if someone adds a test that accidentally sets VINEXT_PRERENDER=1 globally, it could mask a regression. Not blocking — just noting.

[ask-bonk]

The comment explaining the flag semantics is excellent and directly addresses the prior review concern. Well done.

One observation: since resolvedConfig is always { ...cacheLifeProfiles.default, ...profile }, both revalidate and expire will always be defined (the default profile has both). So these two lines are effectively ctx.hasExplicitRevalidate = true; ctx.hasExplicitExpire = true; — the !== undefined checks never fail. This means calling cacheLife() with any argument suppresses both the revalidate and expire nested-dynamic errors, not just the one for the field the user specified.

As the comment notes, this matches Next.js (which tracks the flag at the work unit store level, not per-field). But it means cacheLife({ expire: 60 }) suppresses the revalidate throw even though the user only expressed intent about expire. If per-field suppression is ever desired, these checks would need to inspect the raw user input, not resolvedConfig.

[ask-bonk]

Review posted successfully. Here's a summary of the feedback:

Overall: Good port of Next.js PR #93707. The core logic is correct and the 4-commit iteration shows thoughtful responses to prior review feedback. The implementation matches upstream semantics well.

5 comments posted:

1. effectiveLife.revalidate === 0 check semantics (cache-runtime.ts:747) — The check uses the outer's merged effective life rather than the inner's entry metadata (as Next.js does). The behavior is functionally equivalent in all cases due to the hasExplicit* guards, but the semantic difference is worth documenting for maintainers.

2. parentCtx.lifeConfigs.push(effectiveLife) is a standalone behavioral change (cache-runtime.ts:698) — This fills a gap that Next.js handles via propagateCacheLifeAndTagsToRevalidateStore. It's load-bearing for the error detection and matches Next.js semantics, but it changes caching behavior for existing nested "use cache" setups in production (not just during prerender/dev). Worth calling out as a behavioral change in the commit message, not just an implementation detail.

3. nestedCacheContextPhrase priority order (cache-runtime.ts:75) — When both VINEXT_PRERENDER=1 and NODE_ENV=development are set, the message says "in development" rather than "during prerendering". Suggested checking VINEXT_PRERENDER first for accuracy.

4. Test gate coverage (shims.test.ts:4289) — The production + dev test pair is solid and pins both branches. Noted a minor remaining gap: no explicit test that NODE_ENV=test (Vitest default) doesn't trigger the throw, though it's implicitly covered.

5. hasExplicit* flags are always both true (cache.ts:784) — Since resolvedConfig always includes both fields from the default profile, cacheLife() with any argument suppresses both revalidate and expire errors. Matches Next.js but worth noting for future per-field suppression work.

github run

[james-elicx]

/bigbonk review

[ask-bonk]

This has been through 4 prior review rounds and the iteration quality is evident. The core logic is correct, tests are comprehensive, and the extensive inline documentation is appreciated.

I have one real issue (misleading comment that could cause a future maintainer to remove a necessary guard), one behavioral concern worth confirming, and one minor nit. The rest is LGTM.

[ask-bonk]

This comment is incorrect and could mislead a future maintainer into removing the hasExplicit* guard.

The comment says: "If the outer's own explicit cacheLife() pushed revalidate/expire above the thresholds, the inner's values still appear in lifeConfigs but minimum-wins drops them, the checks below evaluate false, and the captured error is silently dropped."

Minimum-wins doesn't "drop" the inner's values — it takes the minimum. If the outer has revalidate: 60 and the inner has revalidate: 0, then Math.min(60, 0) === 0. The effectiveLife.revalidate === 0 check at line 772 is still true. What actually suppresses the throw is !ctx.hasExplicitRevalidate being false (because the outer called cacheLife()).

The comment should describe what actually happens:

Suggested change

	// This block is tightly coupled with the `lifeConfigs.push(effectiveLife)`
	// above: it relies on the inner's dynamic values being merged into this
	// outer's `effectiveLife` via minimum-wins. If the outer's own explicit
	// cacheLife() pushed revalidate/expire above the thresholds, the inner's
	// values still appear in `lifeConfigs` but minimum-wins drops them, the
	// checks below evaluate false, and the captured error is silently dropped.
	// That is the desired behavior — the outer made an explicit choice that
	// overrides the dynamic child.
	// This block is tightly coupled with the `lifeConfigs.push(effectiveLife)`
	// above: it relies on the inner's dynamic values being merged into this
	// outer's `effectiveLife` via minimum-wins. When the outer has its own
	// explicit `cacheLife()`, the effective life may still be dynamic (e.g.,
	// `Math.min(60, 0) === 0`), but the `hasExplicitRevalidate`/
	// `hasExplicitExpire` guards below suppress the throw — the outer made
	// an explicit choice that opts it out of the error, even though the
	// effective values remain dynamic via minimum-wins.

[ask-bonk]

Previous reviews have flagged this as a standalone behavioral change beyond the error-cause feature, and the PR description acknowledges it. One thing I want to confirm:

Pre-PR, if a nested "use cache" inner resolved revalidate: 0, the outer's effectiveLife was computed only from its own lifeConfigs (i.e., only from cacheLife() calls within the outer's own body). Post-PR, the outer's effectiveLife includes the inner's resolved life via this push, so minimum-wins gives revalidate: 0 even if the outer called cacheLife({ revalidate: 60 }). This changes caching behavior in production (where the throw doesn't fire) — the outer silently stops being cached.

The minimum-wins documenting test at shims.test.ts:4397 explicitly verifies this, which is great. But the commit message for the initial commit (ef0e0e8) frames this as purely an error-cause feature. Consider amending the PR description (or the squash-merge commit message) to call out: "Also propagates nested cache life configs to parent via lifeConfigs.push, matching Next.js's propagateCacheLifeAndTagsToRevalidateStore semantics. This changes minimum-wins behavior for nested caches — the outer's effective cache life now reflects the inner's contribution."

[ask-bonk]

Nit: the sequencing invariant comment is thorough and correct, but it references fn(...args) returning as the guarantee. Worth noting the subtle case: if fn calls multiple nested cached functions in sequence (await innerA(); await innerB()), both inner calls' lifeConfigs.push complete before fn returns (because each await ensures the inner's post-execution runs). But if fn calls them in parallel (await Promise.all([innerA(), innerB()])), the pushes still complete before the outer's resolveCacheLife because await cacheContextStorage.run(ctx, () => fn(...args)) waits for fn's returned promise, which is the Promise.all. So the invariant holds for both sequential and parallel inner calls.

This is already implicitly safe, but a single sentence like "This holds for both sequential and Promise.all-parallel inner calls, because fn's returned promise settles only after all nested awaits complete." would make it explicit for future readers scanning the comment.

[ask-bonk]

Review posted successfully. Here's a summary of the feedback:

Overall: Good port of Next.js PR #93707. The core logic is correct, tests are comprehensive, and the code has been well-iterated across 5 commits based on prior review feedback. LGTM with minor items.

3 comments posted:

1. Misleading comment at cache-runtime.ts:724-731 (the only real issue) — The comment claims that when the outer has explicit cacheLife(), "minimum-wins drops [the inner's values] and the checks below evaluate false." This is wrong. Minimum-wins takes Math.min(60, 0) = 0, so the effective life check is still true. It's the hasExplicit* guard that suppresses the throw, not the effective life going above the threshold. Posted a suggestion to correct the comment, since a future maintainer reading it could mistakenly conclude the hasExplicit* guards are redundant.

2. Behavioral change needs PR description update (cache-runtime.ts:704) — The parentCtx.lifeConfigs.push(effectiveLife) is a standalone behavioral change that affects production caching (not just the error-cause feature). The minimum-wins documenting test at shims.test.ts:4397 correctly verifies this, but the commit message frames it as purely error-cause work. Suggested updating the squash-merge commit message to call this out explicitly.

3. Sequencing invariant comment nit (cache-runtime.ts:691) — The invariant holds for both sequential (await a(); await b()) and parallel (await Promise.all([a(), b()])) inner calls, since fn's returned promise only settles after all nested awaits. Suggested adding a single sentence to make this explicit for future readers.

github run

[james-elicx]

/bigbonk review

[ask-bonk]

Solid port of Next.js PR #93707. The implementation is correct, the PR description clearly documents the behavioral change (lifeConfigs propagation), the tests are comprehensive with both sides of the prerender/dev gate pinned, and the inline documentation is unusually thorough for the complexity involved. The 5 prior review rounds have been addressed well — context-aware error messages, captureStackTrace guard, corrected coupling comment, and dev-mode companion test are all present.

Two minor items below, neither blocking.

[ask-bonk]

Nit: this pushes effectiveLife unconditionally into the parent's lifeConfigs — including when this cache is a top-level call with parentCtx being a non-cache cacheContextStorage.run() scope from a test or from throwIfInsideCacheScope(). In practice this is harmless (the parent contexts created by tests have no downstream consumer of lifeConfigs), but it would be slightly cleaner and more defensive to also check parentCtx.variant !== undefined or some sentinel, to avoid pushing into a context that isn't actually a "use cache" parent.

Not blocking — just noting for future robustness.

[ask-bonk]

The refactoring from two inline resolveCacheLife call sites (one in registerCachedFunction, one in executeWithContext) into a single one in runCachedFunctionWithContext is a net improvement — it eliminates the duplication and ensures both paths get the nested-dynamic error detection.

One thing worth noting: the shared-cache path in registerCachedFunction (line 504) now receives the effectiveLife from runCachedFunctionWithContext and uses it for both recordRequestScopedCacheLife AND the handler.set() call (lines 510, 549-550). Pre-PR, the effectiveLife in the shared-cache path only reflected the outer's own cacheLife() calls. Post-PR, it also includes the inner's propagated life via parentCtx.lifeConfigs.push. This means the revalidateSeconds passed to handler.set() is now the minimum-wins value across both outer and inner.

This is correct behavior (matches Next.js), but it's a second consequence of the lifeConfigs.push change beyond the throw detection — the outer's stored cache entry now respects the inner's dynamic life. The PR description covers the minimum-wins behavior but doesn't explicitly call out that the stored revalidateSeconds in the cache handler also changes. Worth a sentence in the commit message for completeness.

[ask-bonk]

Review posted successfully. Here's a summary:

Verdict: APPROVE

The implementation is correct and well-iterated across 6 commits. The core logic — eager NestedDynamicUseCacheError capture at the inner call site, ??= first-child-wins propagation, hasExplicit* suppression, and prerender/dev-only throw gating — all match the upstream Next.js semantics. The PR description was updated to clearly document the lifeConfigs propagation behavioral change, the tests pin both sides of the dev/production gate, and the inline documentation is thorough.

2 minor comments posted (neither blocking):

1. Unconditional parentCtx.lifeConfigs.push (line 711) — The push happens for any non-null parentCtx, including non-cache contexts from tests or throwIfInsideCacheScope. Harmless in practice, but a variant check would be slightly more defensive.

2. Stored revalidateSeconds also changes (line 697) — The lifeConfigs.push change has a second consequence beyond throw detection: the outer's stored cache entry in handler.set() now respects the inner's propagated life. The PR description covers minimum-wins behavior but doesn't explicitly call out this storage-side effect. Worth a sentence in the commit message.

github run