fix(use-cache): match Next.js erroring searchParams in "use cache" pages (#4028)

* fix(use-cache): fail a read of searchParams.then inside "use cache", like Next.js

The erroring searchParams a public "use cache" page gets threw only when
`then` was called. Next.js's makeErroringSearchParamsForUseCache throws
when `then` is read, so a thenable check such as `typeof sp.then` fails
too. Use the observeThenRead hook added in #4002.

* fix(use-cache): keep page searchParams erroring after they escape "use cache"

Next.js's makeErroringSearchParamsForUseCache throws on every access.
vinext's replacement only threw inside the cache scope, so a value that
escaped the cache function read as an empty query afterwards.

* fix(use-cache): match Next.js for catch, finally and escaped access on page searchParams

Next.js binds catch and finally to the underlying promise, so they never
read `then` through the erroring proxy. Run observeThenRead only on a
read of `then`, and let an empty key list through, so catch, finally and
Object.keys settle as in Next.js.

An access after the value escapes the cache function now records the
invalid use on the request the value was made for, like Next.js's
`workStore.invalidDynamicUsageError ??= error`.

* fix(use-cache): port the erroring page searchParams proxy from Next.js

Use a dedicated get-only proxy like makeErroringSearchParamsForUseCache
instead of makeThenableParams, so `in`, Object.hasOwn and catch/finally
behave as in Next.js, and revert the thenable-params change. Make it inside
the cache scope and record an access on that scope and request, wherever
the value is read later.

* fix(use-cache): keep the first caught searchParams access on the cache scope

Like Next.js's workStore.invalidDynamicUsageError ??= error.

* fix(use-cache): fail a "use cache" that an escaped page searchParams is read in

Record the access on the active cache scope too, so a cache that catches
it can't store a fallback that a later request would reuse.

* fix(use-cache): record escaped searchParams access on the request context

Capture the unified request context, as throwIfInsideCacheScope records
on, instead of the headers shim state. Outside a request that was the
module fallback state, so an access leaked into later renders.

Author: james-elicx

packages/vinext/src/shims/cache-runtime.ts

@@ -48,17 +48,18 @@ import {
   runWithDetachedFetchObservations,
 } from "./fetch-cache.js";
 import { getOrCreateAls } from "./internal/als-registry.js";
+import { isWellKnownProperty } from "./internal/thenable-well-known-properties.js";
+import {
+  createUseCacheAccessError,
+  recordInvalidDynamicUsageError,
+} from "./internal/headers-state.js";
 import {
   isInsideUnifiedScope,
   getRequestContext,
   runWithUnifiedStateMutation,
 } from "./unified-request-context.js";
-import { isDraftModeEnabled, markDynamicUsage, throwIfInsideCacheScope } from "./headers.js";
-import {
-  makeThenableParams,
-  getFallbackParamsSnapshot,
-  restoreThenableParams,
-} from "./thenable-params.js";
+import { isDraftModeEnabled, markDynamicUsage } from "./headers.js";
+import { getFallbackParamsSnapshot, restoreThenableParams } from "./thenable-params.js";
 import {
   createPprFallbackShellSuspensePromise,
   trackPprFallbackShellCacheTask,
@@ -782,19 +783,20 @@ export function registerCachedFunction<TArgs extends unknown[], TResult>(
             outerParams,
           ),
         }) as TArgs;
-        // The encoded args omit a public page's searchParams. Access must still
-        // fail like Next's erroring searchParams, including on a replay.
-        if (pagePropsIndex !== undefined) {
-          callArgs = withErroringPageSearchParams(callArgs, pagePropsIndex) as TArgs;
-        }
       }
+      // The encoded args omit a public page's searchParams. Access must still
+      // fail like Next's erroring searchParams, including on a replay.
+      const callFn =
+        segmentPropsIndex !== -1 && pagePropsIndex !== undefined
+          ? withErroringPageSearchParams(fn, pagePropsIndex)
+          : fn;
 
       if (fallbackParams) {
         // Speculative shell params are incomplete: a placeholder must not hit
         // a concrete route's entry or become an unguarded persisted replay.
         // Keep decoded values plus lazy framework suspension for execution;
         // concrete params still follow the normal cache path below.
-        return (await executeWithContext(fn, callArgs, cacheVariant, rsc)).result;
+        return (await executeWithContext(callFn, callArgs, cacheVariant, rsc)).result;
       }
 
       // "use cache: private" uses per-request in-memory cache
@@ -816,7 +818,7 @@ export function registerCachedFunction<TArgs extends unknown[], TResult>(
           return privateHit.result;
         }
 
-        const execution = await executeWithContext(fn, callArgs, cacheVariant, rsc);
+        const execution = await executeWithContext(callFn, callArgs, cacheVariant, rsc);
         if (execution.cacheable) privateCache.set(cacheKey, execution);
         return execution.result;
       }
@@ -826,7 +828,7 @@ export function registerCachedFunction<TArgs extends unknown[], TResult>(
       // preview request would otherwise seed unpublished content into an entry
       // later served to public requests. Mirrors Next.js's `isDraftMode` guard.
       if (isDev || isDraftModeEnabled()) {
-        return (await executeWithContext(fn, callArgs, cacheVariant, rsc)).result;
+        return (await executeWithContext(callFn, callArgs, cacheVariant, rsc)).result;
       }
 
       // Shared cache ("use cache" / "use cache: remote")
@@ -879,7 +881,7 @@ export function registerCachedFunction<TArgs extends unknown[], TResult>(
       // propagate: the request already used the stale value.
       const generate = async (propagate: boolean): Promise<TResult> => {
         const { result, ctx, effectiveLife, collectedResult } = await runCachedFunctionWithContext(
-          fn,
+          callFn,
           callArgs,
           cacheVariant,
           (value) => serializeCacheResult(value, rsc),
@@ -1464,19 +1466,65 @@ function toReplayablePageArgs(args: readonly unknown[], index: number): unknown[
 }
 
 /**
- * Give page props at `index` without `searchParams` (a Response Store replay of
- * args encoded by `toReplayablePageArgs`) a value that throws on
- * access inside the cache scope, like Next.js's
- * `makeErroringSearchParamsForUseCache`, instead of `undefined`.
+ * Run `fn` with an erroring `searchParams` in the page props at `index` when
+ * they have none (they are omitted from a public page cache's arguments, and
+ * from a Response Store replay of them), instead of `undefined`.
  */
-function withErroringPageSearchParams(args: readonly unknown[], index: number): readonly unknown[] {
-  const props = args[index];
-  if (!isPagePropsObject(props) || "searchParams" in props) return args;
-  return replaceArgument(args, index, {
-    ...props,
-    searchParams: makeThenableParams(
-      {},
-      { observeParamAccess: () => throwIfInsideCacheScope("searchParams") },
-    ),
+function withErroringPageSearchParams<TArgs extends unknown[], TResult>(
+  fn: (...args: TArgs) => Promise<TResult>,
+  index: number,
+): (...args: TArgs) => Promise<TResult> {
+  return (...args) => {
+    const props = args[index];
+    if (!isPagePropsObject(props) || "searchParams" in props) return fn(...args);
+    // Made inside the cache scope, so it can record an access on this scope.
+    return fn(
+      ...(replaceArgument(args, index, {
+        ...props,
+        searchParams: makeErroringPageSearchParams(),
+      }) as TArgs),
+    );
+  };
+}
+
+// Next.js's `wellKnownProperties` (shared/lib/utils/reflect-utils.ts). Unlike
+// `isWellKnownProperty`, `value` and `error` aren't reserved, so reading them
+// throws unless React wrote them onto the promise.
+function isNextWellKnownProperty(prop: string): boolean {
+  return prop !== "value" && prop !== "error" && isWellKnownProperty(prop);
+}
+
+/**
+ * Ported from Next.js's `makeErroringSearchParamsForUseCache`
+ * (server/request/search-params.ts): reading `then` or a param throws, and
+ * everything else reads the underlying promise, with methods bound to it.
+ */
+function makeErroringPageSearchParams(): Promise<Record<string, never>> {
+  // Like Next.js, which closes over the workStore, an access records the
+  // invalid use on the cache scope and request the value was made for, even
+  // after it escapes the cache function.
+  const cacheContext = cacheContextStorage.getStore() ?? null;
+  const requestContext = isInsideUnifiedScope() ? getRequestContext() : null;
+  const promise = Promise.resolve({});
+  return new Proxy(promise, {
+    get(target, prop, receiver) {
+      if (
+        !Object.hasOwn(target, prop) &&
+        typeof prop === "string" &&
+        (prop === "then" || !isNextWellKnownProperty(prop))
+      ) {
+        const error = createUseCacheAccessError("searchParams");
+        if (cacheContext) cacheContext.invalidDynamicUsageError ??= error;
+        // A cache the value escaped into must not store a result either.
+        const activeCacheContext = cacheContextStorage.getStore();
+        if (activeCacheContext && activeCacheContext !== cacheContext) {
+          activeCacheContext.invalidDynamicUsageError ??= error;
+        }
+        if (requestContext) recordInvalidDynamicUsageError(requestContext, error);
+        throw error;
+      }
+      const value: unknown = Reflect.get(target, prop, receiver);
+      return typeof value === "function" ? value.bind(target) : value;
+    },
   });
 }

packages/vinext/src/shims/internal/headers-state.ts

@@ -178,6 +178,18 @@ function propagateInvalidDynamicUsageError(state: VinextHeadersShimState, error:
   });
 }
 
+/**
+ * Record an invalid dynamic API use on `state`, keeping the first one, like
+ * Next.js's `workStore.invalidDynamicUsageError ??= error`.
+ */
+export function recordInvalidDynamicUsageError(
+  state: VinextHeadersShimState,
+  error: unknown,
+): void {
+  state.invalidDynamicUsageError ??= error;
+  propagateInvalidDynamicUsageError(state, error);
+}
+
 export function markRenderRequestApiUsage(kind: RenderRequestApiKind): void {
   getHeadersShimState().renderRequestApiUsage.add(kind);
 }
@@ -240,6 +252,15 @@ export function isInsideAnyCacheScope(): boolean {
   return _getUseCacheGuardContext() !== null || _isInsideUnstableCache();
 }
 
+/** The error a dynamic request API throws inside a public "use cache". */
+export function createUseCacheAccessError(apiName: string): Error {
+  return new Error(
+    `\`${apiName}\` cannot be called inside "use cache". ` +
+      `If you need this data inside a cached function, call \`${apiName}\` ` +
+      "outside and pass the required data as an argument.",
+  );
+}
+
 /**
  * Throw if the current execution is inside a "use cache" or unstable_cache()
  * scope. Called by dynamic request APIs (headers, cookies, connection) to
@@ -249,11 +270,7 @@ export function isInsideAnyCacheScope(): boolean {
  */
 export function throwIfInsideCacheScope(apiName: string): void {
   if (_isInsidePublicUseCache()) {
-    const error = new Error(
-      `\`${apiName}\` cannot be called inside "use cache". ` +
-        `If you need this data inside a cached function, call \`${apiName}\` ` +
-        "outside and pass the required data as an argument.",
-    );
+    const error = createUseCacheAccessError(apiName);
     // Record the error on the request context so it survives user try/catch
     // and can be forwarded to the dev overlay on client-side navigations.
     // Ported from Next.js: workStore.invalidDynamicUsageError assignment in