Skip to content

Follow-ups: caching/PPR/render-pipeline audit (DRY consolidations, deferred coverage, held/investigate items) #684

Closed

Description

@ivogt

Follow-ups from the caching/PPR/render-pipeline audit and the advisor/caching-ppr-fixes change wave (8 fixes: shell-HIT redirect guard, document-cache host key, render-callable cacheTag() docs, Vercel base64 parity, CF no-TTL marker warning, navigation-loading dev+prod e2e parity, untagged-bake + action-refetch diagnostics, SSR capture hardening). Those 8 landed and pass the full gate; the items below were deliberately deferred out of that wave. Plan files with full context live under plans/ (001–011 + README).

Grouped by type. None is blocking; each is independently pickup-able.


A. DRY consolidations (from the /simplify review)

All three are small, apply to files outside the change wave's scope, and were skipped to keep the wave file-disjoint. Each new site correctly mirrors an established idiom — these consolidate the idiom, not fix a defect.

  • safeSameOriginLanding(raw, origin, basename) shared helper. The "neutralize a blocked redirect to a safe same-origin landing" fallback (basename && basename !== "/" ? basename : "/") is inlined in both packages/rangojs-router/src/rsc/redirect-guard.ts:82 and the new resolveShellHitRedirectTarget in packages/rangojs-router/src/rsc/rsc-rendering.ts. Extract one helper (in src/redirect-origin.ts, alongside resolveSameOriginRedirect) so the "where does a blocked redirect go" answer lives in one place. Effort: S.

  • documentKeyBase(url) shared helper. Host+path+search key composition now lives in two near-duplicate spots with slightly different search formatting: packages/rangojs-router/src/cache/cache-scope.ts getCacheKeyBase (${host}${pathname}…) and the inline default-key template in packages/rangojs-router/src/cache/document-cache.ts. Extract a shared base so the host-namespacing rule can't drift between the segment and document tiers. Effort: S.

  • warnOncePerNamespace(seen, message) private method on CFCacheStore. The const id = this.namespace ?? "default"; if (!seen.has(id)) { seen.add(id); console.warn(...) } dance is now open-coded three times in packages/rangojs-router/src/cache/cf/cf-cache-store.ts (the two pre-existing floor/read-side warnings + the new no-TTL one). Collapse to a private method. Effort: S.

  • Migrate warn-once console emitters to a structured diagnostics surface when the operability surface from PPR operability: snapshot size guard, capture diagnostics, and the silent missing-KV no-op #651/PPR capture pipeline hardening: narrow the write barrier, unify bounded drains, tame the backoff-vs-CI flake #652 lands. Do all of them as one batch: the two new emitters (warnedNoTagInvalidationTtl in cf-cache-store, warnUntaggedShellBakeOnce in shell-capture) plus their four existing siblings (warnedNoKvReadInvalidation, warnedTagInvalidationTtlFloor, warnedNullCaptures, warnedRefusedCaptures). Neither new warning is a special candidate to move alone. Depends on: PPR operability: snapshot size guard, capture diagnostics, and the silent missing-KV no-op #651/PPR capture pipeline hardening: narrow the write barrier, unify bounded drains, tame the backoff-vs-CI flake #652.

B. Deferred test coverage

  • Shell-HIT redirect flip-to-redirect e2e (dev + production). Plan 001 shipped the fix (resolveShellHitRedirectTarget) + a unit test on the extracted helper, but deferred the end-to-end case: capture a PPR shell, flip the route to match.redirect within the shell TTL, and assert a cross-origin flip lands on the safe same-origin target — in BOTH dev and (production). Needs a switchable-redirect route in packages/rangojs-router/e2e/test-app (+ its .gen.ts). The branch is documented as near-unreachable, so this is thoroughness coverage of the guard, not a hot path. See plans/001-shell-hit-redirect-guard.md Step 3. Effort: M (fixture design is the cost).

C. Held / investigate items

  • [HELD — needs design] Hole-free shell-HIT loader optimization (plan 007). Original idea: skip the fresh bake-lane loader run on hole-free shell HITs since the overlay discards its result. An adversarial fact-check proved this UNSAFE as a skip — executeLoaderData on the HIT path has unconditional side effects the system relies on: loader telemetry (phase metric + rango.loader span via ctx.use → observePhase), cache-tag recording into _requestTags, and the _loaderCacheOverrides interceptor install that lets a handler's ctx.use(loader) get the overlaid value instead of re-executing. A real win is a capture-time-replay redesign (record tags/metrics/override at capture, replay on HIT without running the body), not a surgical skip. This is a design spike, not an executor task. Full reasoning in plans/007-hole-free-shell-hit-skip-loader.md.

  • [INVESTIGATE] Capture derived-context render-barrier inheritance (plan 009). attemptCapture's derivedCtx = Object.create(reqCtx) resets eight render-scoped accumulators but NOT the _renderBarrier* family, and the barrier getter/resolver are closure-bound to the FOREGROUND ctx — so a bake-lane loader calling ctx.rendered() then ctx.use(handle) during capture reads the foreground request's handle snapshot, and the capture lane is coupled to the foreground's single barrier instance. Reachable only via rendered() in a bake-lane loader (experimental surface). Deliverable is a verdict + reproduction (or documented non-repro), not a speculative fix. Note: this touches src/rsc/shell-capture.ts, which fix(ppr): collect shell tags at the putShell write barrier — async-recorded tags evict the shell (#676) #680 already restructured. See plans/009-investigate-capture-barrier-inheritance.md.

  • [INVESTIGATE] SWR background handle-store swap (plan 010). On a stale "use cache" HIT, the background revalidation swaps the SHARED requestCtx._handleStore and stamps stampCacheExec on shared args, restoring only in a finally — and waitUntil schedules on the next microtask, so the prologue can run while the foreground is still rendering. Hypotheses: (a) foreground handle pushes get routed into the isolated background store and lost from the live document; (b) a concurrent foreground ctx.set()/ctx.header() throws INSIDE_CACHE_EXEC. The sibling background-revalidation.ts swaps only AFTER draining the foreground — the "safe ordering" reference. Deliverable is a verdict + reproduction per hazard. See plans/010-investigate-swr-bg-handle-store-swap.md.

  • [INVESTIGATE] SSR resume errors are unrecoverable after prelude commit (SSR-03). resumeShellHTML runs JSON.parse(postponed) / resume() at packages/rangojs-router/src/ssr/index.tsx AFTER rsc-rendering.ts has already enqueued preludeBytes (committed HTTP 200 + static shell). A corrupt/truncated stored postponed blob or any resume() throw yields a permanently broken page for that cache key until it ages out, with no fallback to axis-1. Fix direction: parse + start resume() setup (or an integrity check on the postponed blob) BEFORE the serve path commits the prelude. Effort: M. Interacts with SSR-04.

  • [INVESTIGATE] Shell cache invalidated only by React.version, not app build/generation (SSR-04). buildShellKey (packages/rangojs-router/src/rsc/shell-serve.ts) keys on host+path+search only, and isValidShellHit gates solely on entry.reactVersion === React.version. With a persistent shared store (KV / runtime-cache / D1) that survives deploys, an app-code change that keeps the same React version leaves a stale-build prelude+postponed live under the same key → resuming an old build's postponed holes against new Flight → tree-mismatch in resume() (the SSR-03 broken-serve). Fold a build/generation id (or ctx.version) into the key or the validity check. Effort: M. Confirm overlap with PPR operability: snapshot size guard, capture diagnostics, and the silent missing-KV no-op #651/PPR: capture-time taint guard — refuse to store shells that read per-request data into shell content #653 first.

D. Tooling

  • Extend an API-existence guard to the caching skills. pnpm check:docs-api guards only docs/why-rango.md. The cacheTag() render-callable drift fixed in plan 003 (skills teaching a "use cache"-only API after the code shipped a second form) recurred silently because the caching skills aren't guarded. Extend the check to the caching/PPR skills so referenced identifiers/signatures are verified against src/. Effort: M.

Deferred-and-rejected items (recorded so they aren't re-audited): the 32-bit param-hash / cache-key collision ceilings (PPR-03, CACHE-04) are documented in-code as bounded tradeoffs with explicit revisit triggers; the refused-capture backoff map retention (PPR-04) is intentional escalation, folded into #652. See plans/README.md.

Activity

  1. ivogt commented on Jul 5, 2026

    @ivogt
    CollaboratorAuthor

    Change wave landed in #685 (branch advisor/caching-ppr-fixes, 8 commits). The follow-ups tracked here are the items deliberately deferred out of that PR.

  2. ivogt commented on Jul 5, 2026

    @ivogt
    CollaboratorAuthor

    Status update. Groups A + D are implemented in draft PR #686 (shared safeSameOriginLanding, shared cacheKeyBase, warnOncePerNamespace, docs-api guard over the 8 caching/PPR skills — the guard shakedown also fixed a class-declaration false-negative that hid Skip). A4 stays blocked on #651/#652. Groups B and C were investigated; verdicts below, each independently reproduced or traced with path:line evidence.

    B. Shell-HIT redirect flip e2e — NOT DETERMINISTICALLY REACHABLE, recommend closing as unit-covered

    The planned fixture (capture shell → flip route to redirect → HIT within TTL) cannot fire the branch through real routing, in either mode:

    • A request on a now-redirecting URL classifies as plan.mode === "redirect", which leaves _classifiedRoute unset (rsc/handler.ts:827), which nulls resolvePprConfig at the shell gate (rsc/rsc-rendering.ts:124) — the stored shell entry is never consulted; the request exits as a plain 3xx through guardOutgoingRedirect.
    • Within one request, the tail's match() reuses the classification snapshot (router/match-api.ts:67-69), so classification and tail can only disagree via a mid-request dev-HMR invalidation race — not deterministically testable.
    • match.redirect is only ever produced by URL normalization (trie-matching.ts:325-343, pattern-matching.ts:484-568) — always same-origin, which is why the branch comment says near-unreachable.

    The branch is correct defense-in-depth and its neutralization matrix is fully pinned by src/rsc/__tests__/shell-hit-redirect-guard.test.ts (cross-origin → root/basename, protocol-relative, same-origin pass-through). A monkey-patched e2e would test the mock, not the router. Proposing to check this box off as unit-covered with this analysis as the record.

    C. Investigation verdicts

    Plan 009 — capture barrier inheritance: CONFIRMED (survived #680/#685). attemptCapture's derived ctx (shell-capture.ts:748-770) resets 8 accumulators but inherits the whole _renderBarrier* family; the barrier is closure-bound to the foreground ctx (request-context.ts:1061-1107), and its resolver no-ops once resolved — so the capture lane never gets its own barrier. Reproduced against production wiring (real createRequestContext + setupLoaderAccess): a bake-lane loader's await ctx.rendered(); ctx.use(handle) resolves instantly and reads the FOREGROUND handle snapshot; the capture's fresh _handleStore is invisible. 2/2 repro tests green. Consequence: baked loader values can embed foreground handle data inside a capture-lane shell — an internal inconsistency whenever an uncached handler pushes non-deterministic handle data. Fix: extract the barrier wiring into a helper and wire the derived ctx its own barrier + reset _renderBarrierSegmentOrder/Waiters/HandleSnapshot/GuardClosed/_handlerLoaderDeps/_treeHasStreaming. Effort S code / M with mandated coverage. One un-repro'd corner (code-read only): with _treeHasStreaming inherited true, a capture-lane rendered() can seal() the capture's fresh store mid-capture, pairing it with the foreground's segment order.

    Plan 010 — SWR background handle-store swap: BOTH HAZARDS CONFIRMED, plus a third. Swap site is cache/cache-runtime.ts:429-430 (stale-hit branch), stamp :459-464, restore-in-finally :510-517; production waitUntil runs the prologue on the next microtask (request-context.ts:996), while the foreground is mid-render. Unit repro with production-faithful scheduling (the existing suite's serializing waitUntil mock is exactly why it never saw this), all green in one run: (a) foreground handle push lands in the background's isolated store — lost from the live document; (b) foreground ctx.set() throws INSIDE_CACHE_EXEC during the window; (c) NEW: the misrouted foreground push is persisted into the background's cache entry (cross-contamination). Note two existing tests PIN the hazardous behavior (cache-runtime-stale.test.ts:259, :478). Fix: derived-context isolation (the exact pattern shell-capture.ts:748-753 uses) or drain-then-swap like background-revalidation.ts:129-155. Effort M. This is the highest-priority C item — real bug, not theoretical.

    SSR-03 — resume errors after prelude commit: CONFIRMED (not addressed by #685 — that hardening was capture-side only). resumeShellHTML's only failure handling is report-and-rethrow (ssr/index.tsx:638-646); the rejection lands in controller.error (rsc-rendering.ts:577) AFTER the 200 + full static prelude flushed. Client gets a visually-complete page that never hydrates (holes stuck on fallbacks, no Flight payload). No eviction exists (deleteShell appears nowhere; store family is get/put only) and failure triggers no recapture — every request re-serves the broken entry until TTL (default 300s, consumer-set can be hours). Corrupt-prelude variant (base64ToBytes, bare atob at shell-serve.ts:88-93) throws pre-commit instead — 500-path, equally no degrade, equally repeating. Fix (S code / M with coverage): extend the HIT validity gate (rsc-rendering.ts:180) to try base64ToBytes(prelude) + JSON.parse(postponed) and fall through to the existing MISS path (axis 1 + recapture overwrites the corrupt entry); additionally schedule a recapture in the tail catch so parseable-but-mismatched blobs self-heal instead of persisting the TTL. Moving resume() itself pre-commit would forfeit the streaming design — don't.

    SSR-04 — shell validity ignores build id: CONFIRMED-GAP (engine), PARTIALLY MITIGATED (default CF only). Gate is entry.reactVersion === React.version only (shell-serve.ts:83-85); the ShellCacheEntry doc at cache/types.ts:227-229 even promises "never mixed across … a build change" — documented, unenforced. Store reality: CFCacheStore's v/${version}/ key prefix rotates per deploy (mitigated unless a consumer pins options.version); VercelCacheStore has NO default version (vercel-cache-store.ts:361) — naive getCache() wiring serves stale-build shells across deploys, feeding exactly the SSR-03 broken-serve; named memory stores survive dev HMR via the globalThis registry. Fix (S, and cheaper than key-folding): stamp buildVersion: ctx.version (the @rangojs/router:version build stamp, already read in the same function at rsc-rendering.ts:256) into the entry at capture and extend isValidShellHit — mismatch = miss = recapture overwrites the same key (self-healing, same doctrine as the reactVersion gate; key-folding would orphan dead entries + double-version on CF). Entries without the field = miss (pre-release, no shim). No overlap with #651/#653 beyond #651's debug sink being the natural place for a mismatch outcome. Sequencing: SSR-03 + SSR-04 + plan 009 all touch the same gate/capture area — one PR for the pair SSR-03/04 is the efficient cut.

    Plan 007 — hole-free shell-HIT replay: NO-GO (design spike complete, safety analysis holds, numbers don't). Key traced fact: for a CACHED bake-lane loader the HIT tail already never runs the body (SeededShellStore pins the item-family record — shell-snapshot.ts:293-301, loader-cache.ts:279-293). The only body that runs on a hole-free HIT is an UNCACHED bake-lane loader — the exact shape the untagged-bake warning (shell-capture.ts:330-361) already discourages, with a one-line user remedy (cache the loader). TTFB is untouched either way (prelude flushes first). Full record-at-capture/replay-on-HIT design (what to record, storage, degrade rules, tag-attribution hazard) is preserved in the spike output should the reopen trigger fire: real-app profiling showing hole-free HIT tails dominated by slow uncached bake-lane loaders that genuinely can't be cached. Sequencing dependency: any future implementation goes AFTER SSR-04's build-id gate. Cheap interim: a /ppr skill note saying "cache your bake-lane loaders" is the sanctioned way to get this win today.

    Suggested next steps

    1. Land chore(router): #684 follow-ups — DRY consolidations + docs-api guard over caching/PPR skills #686 (A + D).
    2. New PR: SSR-03 + SSR-04 together (validity-gate integrity check + buildVersion stamp + recapture-on-tail-failure) — S/M, same files.
    3. New PR: plan 010 SWR derived-context fix — M, highest severity (confirmed data-affecting bug); requires re-pointing the two pinning tests to production-faithful waitUntil scheduling.
    4. New PR: plan 009 barrier wiring for the capture ctx — S/M, experimental surface, lower urgency.
    5. Check off B with the reachability analysis above; check off 007 as NO-GO with reopen trigger.
  3. ivogt commented on Jul 5, 2026

    @ivogt
    CollaboratorAuthor

    SSR-03 + SSR-04 are implemented in draft PR #687 (buildVersion validity gate + pre-commit payload integrity check + tail-failure healing recapture; red-before-green 8→0, dev+prod e2e via a test-app fault injector). Remaining from this issue: plan 010 (SWR handle-store swap — next up), plan 009 (capture barrier wiring), and the A4 diagnostics batch blocked on #651/#652.

  4. added 2 commits that reference this issue on Jul 5, 2026
  5. ivogt commented on Jul 5, 2026

    @ivogt
    CollaboratorAuthor

    Plan 010 is implemented in draft PR #689: the stale-hit background revalidation now runs under a derived context (own _handleStore, shell-capture pattern) instead of swapping the shared store, and no longer stamps INSIDE_CACHE_EXEC onto shared args (the miss-path stamp owns in-fn misuse detection; invariant documented at the stamp). Red-before-green 2→20 with a production-faithful microtask-scheduling race test; /simplify ran pre-push (3 findings applied). Remaining here: plan 009 + the A4 diagnostics batch (blocked on #651/#652).

  6. ivogt commented on Jul 5, 2026

    @ivogt
    CollaboratorAuthor

    Status: #687 (SSR-03 + SSR-04, squash a1cae548) and #689 (plan 010, squash 518a9e74) are both MERGED to main, each with a fully green non-draft CI matrix. The dev-shard flake both PRs tripped over during the runs is documented and mechanism-pinned in #690 (inception-level, unrelated to any recent PPR work). Remaining from this issue: plan 009 (capture barrier wiring for the derived ctx) and the A4 warn-once→diagnostics batch (blocked on #651/#652). B and 007 dispositions recorded above.

  7. ivogt commented on Jul 6, 2026

    @ivogt
    CollaboratorAuthor

    Plan 009 is implemented in draft PR #704: the shell capture's derived context now gets its own render barrier via a shared wireRenderBarrier(ctx, handleStore) helper (extracted from createRequestContext, called from deriveShellCaptureContext — covers both shell producers), with the six barrier-family fields reset as own properties. A capture-lane bake-lane loader's await ctx.rendered(); ctx.use(handle) now reads the CAPTURE's handle store instead of the foreground snapshot, and _treeHasStreaming is recomputed for the capture's tree (the streaming corner — premature seal of the capture's fresh store — is covered by the same reset). Red-before-green 3→0 in a new suite driving the real match path under the real derivation; full gate green (typecheck 16/16, test:unit:all, lint, format). Remaining from this issue: only the A4 warn-once→diagnostics batch, blocked on #651/#652.

  8. ivogt commented on Jul 6, 2026

    @ivogt
    CollaboratorAuthor

    Status: #704 (plan 009, squash cc830514) is MERGED to main with a fully green non-draft CI matrix — all dev and production e2e shards passed on the run (no #690 flake this time). With that, every actionable item from this issue is closed out: A+D (#686), SSR-03/04 (#687), plan 010 (#689), plan 009 (#704); B and 007 dispositions recorded above. The only remainder is the A4 warn-once→diagnostics batch, which stays blocked on #651/#652 — tracking it there is probably cleaner than keeping this issue open for it.

  9. ivogt commented on Aug 30, 2026

    @ivogt
    CollaboratorAuthor

    All actionable items from this tracker are already on main, as the 2026-07-06 status comment recorded.

    A+D (#686): shared safeSameOriginLanding (src/redirect-origin.ts), shared cacheKeyBase (src/cache/cache-key-utils.ts), warnOncePerNamespace on CFCacheStore. SSR-03/04 (#687), plan 010 (#689), plan 009 (#704). Group B (shell-HIT redirect e2e) is not deterministically reachable through real routing — unit-covered in src/rsc/__tests__/shell-hit-redirect-guard.test.ts. Plan 007 (skip hole-free HIT loaders) was investigated unsafe.

    The leftover A4 warn-once→diagnostics batch was parked on #651/#652; #651 is closed. Not keeping this issue open for that cleanup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions