feat(ppr): add PPR fallback-shell render lifecycle tests

[NathanDrake2406]

Stack position

PR 3 of the original cacheComponents PPR root-param fallback-shell stack.

#1702 and #1714 are now merged, so this PR is rebased onto current main and should be reviewed as the next standalone delta: fallback-shell render lifecycle.

Remaining follow-ups in the stack:

1. feat(ppr): add PPR fallback-shell render lifecycle tests #1715: fallback-shell render lifecycle
2. feat(ppr): add safe serving tests for exact cache HIT and static-param validation #1716: safe runtime serving

Problem

The intended invariant is not “wait for the whole SSR tree.”

For a root-param fallback shell, vinext should wait for public "use cache" work that can complete for known params, but it should not block forever on work that depends on fallback params or request APIs such as headers() / cookies().

Known scoped params must remain synchronously readable. Fallback params should suspend only when they are actually accessed.

What changed

• Adds a fallback-shell prerender scope with explicit lifecycle state.
• Tracks public cache work that fallback-shell prerendering should wait for.
• Stops waiting for work that suspends on:
  • fallback params
  • request-bound dynamic APIs such as headers() / cookies()
• Keeps independent child public cache work able to complete.
• Teaches thenable params to suspend lazily for fallback params while keeping known params synchronously readable.

Contract covered

During fallback-shell prerendering:

• public cache work can hold the shell open until it resolves
• request-bound dynamic work remains behind Suspense
• fallback params suspend only on access
• known params remain sync-readable for layout/root-param content

This keeps the root-param shell invariant narrow: cached known-param content can be ready in the shell without converting the whole document into an allReady wait.

Deliberate non-goals

• No runtime shell cache lookup.
• No exact-cache/static-param dispatch policy.
• No stale shell regeneration.
• No HTML metadata rewrite.

Those remain isolated in #1716.

Validation

After rebasing onto current upstream/main:

• vp test run tests/ppr-fallback-shell.test.ts tests/thenable-params.test.ts
• vp check

References

• Next.js CacheSignal
• Next.js cacheComponents warmup/final render flow
• Next.js fallback params in request params

References #1359

[pkg-pr-new]

Open in StackBlitz

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

commit: 412fc36

[NathanDrake2406]

Replacing with stacked version

[james-elicx]

/bigbonk review

[ask-bonk]

Reviewed the fallback-shell render-lifecycle shim and the thenable-params integration. The cache-task tracking state machine (trackPprFallbackShellCacheTask / ignoreCacheTask / scheduleCacheReadyIfSettled) is well thought out — the ALS task-stack approach correctly stops waiting for cache work that suspends on fallback params while letting independent child cache work complete, and the counter is guarded against double-decrement (task.isPending). Tests pass locally (vp test run tests/ppr-fallback-shell.test.ts tests/thenable-params.test.ts → 25 passed).

Since the new lifecycle exports (trackPprFallbackShellCacheTask, waitForPprFallbackShellCacheReady, preparePprFallbackShellFinalRender, isPprFallbackShellAbortError) are not yet wired into any production render path (only thenable-params.ts consumes createPprFallbackShellSuspensePromise / getPprFallbackShellState), the runtime impact is gated behind runWithPprFallbackShellState, which today only tests invoke. That matches the stated non-goals, so the comments below are mostly about the contract this PR establishes for #1716 to build on.

Main concern: the thenable-params integration makes side effects and await params behavior eager rather than per-access, which diverges from both the PR's stated contract ("suspend only when actually accessed") and from Next.js. Details inline.

[ask-bonk]

await params will hang for any route that has fallback params, even when only known params are read.

Replacing the base promise with the hanging fallbackShellPromise means await params no longer resolves to the plain object during fallback-shell prerendering — it hangs until abort. But the common server-component pattern is const { locale } = await params, where locale is a known root param. Under this change that awaits the hanging promise and never reaches the destructure.

The existing contract (tests at lines 9-17, 72-93, 95-108) is that await params resolves. The new test only covers synchronous Reflect.get(params, "slug"), so this regression isn't caught.

In Next.js (see the params.ts#L197-L367 reference in the PR description), awaiting params during fallback prerender resolves to a proxy; only accessing a fallback key on the resolved object throws the hanging promise. Suggest keeping the base promise resolvable and only throwing on fallback-key access:

Suggested change

	const promise = fallbackShellPromise ?? Promise.resolve(plain);
	const promise = Promise.resolve(plain);

If the resolved object also needs to suspend on fallback-key access, that should be done by wrapping the resolved value, not by hanging the whole promise. Please add a test for const { locale } = await params returning the known param without hanging.

[ask-bonk]

Eager construction triggers lifecycle side effects before any fallback param is accessed.

createPprFallbackShellSuspensePromise is called here at makeThenableParams construction time, and it has two non-lazy side effects: it sets state.hasDynamicBoundary = true and calls ignoreCacheTask(...) on every task in the current ALS stack. So merely building the params object for a route that has a fallback param marks the shell as having a dynamic boundary and stops waiting for in-scope cache work — even if the component only ever reads the known root param and never touches the fallback key.

That contradicts the PR's stated contract ("fallback params suspend only on access" / "suspend lazily"). Consider constructing the suspense promise lazily on first fallback-key access instead, so the dynamic-boundary marking and cache-task ignoring only happen when a fallback param is genuinely read. A null test asserting state.hasDynamicBoundary === false when only locale is accessed would lock this in.

[ask-bonk]

The cleanup function returned by scheduleAfterTask is discarded here, so a scheduled abort timer can't be cancelled. In practice preparePprFallbackShellFinalRender sets isAbortScheduled = false and resets hasDynamicBoundary, and the inner guard re-checks current state before aborting, so a stale timer firing after reset is a no-op rather than a wrong abort. Still, this leaves an uncancellable pending timer per scheduled abort. Consider storing the cleanup (e.g. alongside pendingCacheReadyCleanup) and clearing it in preparePprFallbackShellFinalRender, matching how cache-ready cleanup is handled.

[ask-bonk]

These await delay(5) + expect(isReady).toBe(false) assertions rely on the negative case (the resolver genuinely hasn't been called), so they should be stable. But the cache-ready settle path uses two nested setTimeout(0) (scheduleAfterTask), and the positive-case tests that do depend on it firing within a fixed window could be slow under CI load. The tests that await readyPromise directly (e.g. line 60's test) are more robust. No change required, just flagging the timer dependency in case of intermittent CI flakes.

[ask-bonk]

Review submitted successfully. Here's a summary of what I found in PR #1715.

Review summary

The PR adds a fallback-shell prerender lifecycle shim (shims/ppr-fallback-shell.ts) plus thenable-params integration. The core cache-task tracking state machine is solid, and all 25 tests pass locally. Since the new lifecycle exports aren't wired into any production render path yet (only tests invoke runWithPprFallbackShellState), the runtime impact is currently gated — matching the stated non-goals.

Findings posted (4 inline comments)

1. await params hangs for fallback-shell routes (highest priority) — thenable-params.ts:121 replaces the base resolvable promise with the hanging promise. This breaks the common const { locale } = await params pattern where locale is a known root param: it now awaits a promise that never resolves. The existing contract (multiple tests) is that await params resolves, and the new test only covers sync Reflect.get access, so this isn't caught. Diverges from Next.js, where awaiting resolves to a proxy and only fallback-key access suspends. Suggested keeping the base promise resolvable.

2. Eager lifecycle side effects — createPprFallbackShellSuspensePromise is called at construction time, so hasDynamicBoundary = true and cache-task ignoring fire before any fallback param is accessed. Contradicts the PR's "suspend only on access" contract.

3. Leaked timer cleanup in scheduleAbortIfReady — the cleanup fn is discarded, leaving an uncancellable timer. Low severity (inner guard re-checks state), but inconsistent with how cache-ready cleanup is handled.

4. Timer-based test stability note — flagged the nested setTimeout(0) dependency for potential CI flakes; negative-case assertions are stable, positive cases that await readyPromise directly are more robust.

The state-machine work is genuinely good; the main actionable issue is the await params divergence that should be fixed (or explicitly justified) before this contract is built upon in #1716.

github run

[NathanDrake2406]

Review feedback addressed

1. await params hangs for fallback-shell routes

Fixed. The base promise is now always Promise.resolve(plain), so await params resolves to the plain object even when fallback params exist. The hanging promise is only thrown on synchronous access to a fallback key (via get / getOwnPropertyDescriptor). Added a test verifying await params resolves correctly and known params remain readable.

2. Eager construction triggers lifecycle side effects

Fixed. createPprFallbackShellSuspensePromise is now deferred to the first actual fallback-key access via getFallbackShellPromise(). The side effects (hasDynamicBoundary = true and cache-task ignoring) only fire when a fallback param is genuinely read. Added a test asserting state.hasDynamicBoundary === false after accessing only known params, and true after accessing a fallback param.

3. Leaked timer cleanup

Fixed. Added pendingAbortCleanup to PprFallbackShellState. scheduleAbortIfReady now stores the returned cleanup function, and preparePprFallbackShellFinalRender cancels it before resetting state. This matches the existing pendingCacheReadyCleanup pattern and prevents leaking uncancellable timers.

4. Timer-based test stability note

Acknowledged, no code change. The negative-case assertions (expect(isReady).toBe(false)) rely on the resolver not having been called, so they are stable. The positive cases that await readyPromise directly are robust because they resolve via the cache-ready resolver, not timer races.

All targeted tests pass (vp test run tests/ppr-fallback-shell.test.ts tests/thenable-params.test.ts -> 27 passed) and vp check is clean.

[james-elicx]

/bigbonk review

[ask-bonk]

Review summary

The three actionable items from the prior ask-bonk review are all properly addressed, and I verified the fixes against the current checkout:

1. await params hang — Fixed. makeThenableParams now does Promise.resolve(resolvedParams) (thenable-params.ts:225), so await params resolves to a proxy and only fallback-key access on the resolved object throws the hanging promise. Matches Next.js semantics.
2. Eager lifecycle side effects — Fixed. createPprFallbackShellSuspensePromise is now deferred behind getFallbackShellPromise() (thenable-params.ts:211-216), so hasDynamicBoundary = true / cache-task ignoring only fire on actual fallback-param access. The new tests at thenable-params.test.ts:230 and :246 lock this in.
3. Leaked abort timer — Fixed. pendingAbortCleanup is stored in scheduleAbortIfReady and cancelled in preparePprFallbackShellFinalRender (ppr-fallback-shell.ts:97, :215), matching the pendingCacheReadyCleanup pattern.

Verified locally: vp test run tests/ppr-fallback-shell.test.ts tests/thenable-params.test.ts → 29 passed, and vp check on the four changed files is clean (format + lint + types). I also confirmed the fallback-param getOwnPropertyDescriptor accessor does not violate proxy invariants — spread/JSON.stringify correctly propagate the hanging promise rather than throwing a TypeError.

The cache-task state machine remains well structured. The lifecycle exports are still only invoked by tests (gated behind runWithPprFallbackShellState), matching the stated non-goals for #1716.

One minor contract note for #1716 below; not blocking.

[ask-bonk]

Contract note for #1716 (not blocking): fallbackParamNames is captured at construction time, but getFallbackShellPromise() re-reads the ALS state lazily via createPprFallbackShellSuspensePromise → getPprFallbackShellState(). If a fallback-param object escapes the shell ALS scope and a fallback key is then accessed, getPprFallbackShellState() returns null, so createPprFallbackShellSuspensePromise returns null, and if (p) throw p falls through — silently returning the raw placeholder (e.g. "[slug]") here, and undefined from the getOwnPropertyDescriptor accessor at line 151-154. Two slightly different fallthrough values for the same out-of-scope condition.

For a tests-only PR this is purely theoretical, but when this is wired into a real render path in #1716, an escaped params object reading a fallback key after the shell scope exits won't suspend and won't error — it will leak the [slug] placeholder. Worth a deliberate decision (assert in-scope, or normalize the fallthrough) before the runtime serving layer depends on this.

[ask-bonk]

fatal: could not read Username for 'https://github.com': No such device or address

github run

[ask-bonk]

@james-elicx Bonk workflow failed. Check the logs for details.

View workflow run · To retry, trigger Bonk again.

[james-elicx]

/bigbonk review

[ask-bonk]

Review summary

Verified the current checkout: vp test run tests/ppr-fallback-shell.test.ts tests/thenable-params.test.ts → 29 passed, and vp check on all 4 changed files is clean (format + lint + types). The three actionable items from the prior review are confirmed fixed:

1. await params no longer hangs — makeThenableParams now does Promise.resolve(resolvedParams) (thenable-params.ts:225); awaiting resolves to a proxy and only fallback-key access suspends. Verified Promise.resolve treats the proxy as non-thenable (its then resolves to undefined), so the awaited value is the proxy itself.
2. Lazy lifecycle side effects — createPprFallbackShellSuspensePromise is deferred behind getFallbackShellPromise() (thenable-params.ts:211-216); hasDynamicBoundary/cache-task-ignoring only fire on real fallback-param access. Locked in by the new hasDynamicBoundary === false tests.
3. Abort timer leak — pendingAbortCleanup stored in scheduleAbortIfReady and cancelled in preparePprFallbackShellFinalRender.

The cache-task state machine is sound: I traced the ALS task-stack so a parent suspending on a fallback param is ignored while an independent child cache task still completes and settles cache-ready (confirmed by the childCompleted test). Proxy invariants check out — spread/Object.keys/getOwnPropertyDescriptor on the fallback-key proxy don't throw TypeError and correctly route suspension through the get trap.

Lifecycle exports remain test-only (gated behind runWithPprFallbackShellState), matching the stated non-goals for #1716.

Two non-blocking notes for #1716 below — both only matter once this is wired into a real render path.

[ask-bonk]

Non-blocking (#1716): Inconsistent fallthrough between the get and getOwnPropertyDescriptor traps for the same out-of-scope condition.

When a fallback-param object escapes the shell ALS scope and a fallback key is then read, getFallbackShellPromise() → createPprFallbackShellSuspensePromise() → getPprFallbackShellState() returns null, so the promise is null and if (p) throw p falls through. The get trap then returns Reflect.get(plain, prop) (the raw "[slug]" placeholder), but this getOwnPropertyDescriptor accessor returns undefined (the getter has no return). Same out-of-scope condition, two different observable values, and neither suspends.

For a tests-only PR this is theoretical, but once #1716 wires this into a render path an escaped params object reading a fallback key after the shell scope exits will silently leak the [slug] placeholder instead of suspending or erroring. Worth a deliberate decision (assert in-scope, or normalize the fallthrough) before the serving layer depends on it.

[ask-bonk]

Non-blocking (#1716): During warmup, createPprFallbackShellSuspensePromise ignores in-scope cache tasks (via ignoreCacheTask above) but does not call scheduleCacheReadyIfSettled itself — it relies on the completeCacheTask chain inside ignoreCacheTask to schedule the settle. That works when there's at least one in-scope task to ignore, but if pprFallbackShellCacheTaskStackAls.getStore() is empty at this point (a dynamic API accessed outside any tracked cache task), warmup cache-ready won't be re-scheduled by this call. In the current tests cache-ready is always driven by a tracked task settling, so this is latent. When #1716 drives real warmup renders, confirm a bare headers()/cookies() access with no surrounding cache task still settles the warmup waitForPprFallbackShellCacheReady correctly.

[ask-bonk]

fatal: could not read Username for 'https://github.com': No such device or address

github run