[james-elicx]

Summary

• bound the App Router segment prefetch cache under memory pressure
• keep low-priority prefetch queue slots occupied until RSC bodies are consumed
• add focused regression coverage for cache eviction and queue-slot release

Validation

• vp test run tests/link-navigation.test.ts tests/prefetch-cache.test.ts
• vp check --fix packages/vinext/src/shims/link.tsx packages/vinext/src/shims/navigation.ts tests/link-navigation.test.ts tests/prefetch-cache.test.ts
• REPO="$(pwd)" NEXTJS_DIR="/Users/jamesanderson/Developer/vinext/.nextjs-ref" NEXT_TEST_CONCURRENCY=1 ./scripts/run-targeted-nextjs-e2e.sh test/e2e/app-dir/segment-cache/memory-pressure/segment-cache-memory-pressure.test.ts

[james-elicx]

/bigbonk review for issues

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/@vinext/cloudflare@2466

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

commit: c2d194e

[github-actions]

Example	Preview	Production	Original
app-router-cloudflare	preview	production
pages-router-cloudflare	preview	production
app-router-playground	preview	production	original
realworld-api-rest	preview	production
nextra-docs-template	preview	production
benchmarks	preview	production
hackernews	preview	production	original
workers-cache	preview	production
web	preview	production

[ask-bonk]

/bigbonk review — PR #2466: evict segment prefetches under memory pressure

Reviewed the byte-budget LRU rework, the fetch-slot queue, and the segment-prefetch header changes. The design is sound and I verified the two headline behaviors (byte LRU eviction + queue-slot release) hold together. Tests pass locally (prefetch-cache, link-navigation) and vp check is clean on the two shims. A few issues worth addressing before merge, none of them blocking on their own.

1. Slot-release symbol only released on snapshot/reject — verify every response consumer path (correctness)

scheduleAppPrefetchFetch occupies a defaultAppPrefetchQueue slot (activeDefaultAppPrefetchRequests) and only frees it via release() when either the fetcher rejects, or snapshotRscResponse's finally runs. That covers the current callers:

• prefetchRscResponse ok-path → await snapshotRscResponse(response) (releases in finally) ✅
• prefetchRscResponse not-ok path → explicit releaseAppPrefetchFetchSlot(response) at navigation.ts:875 ✅
• fetcher rejection → release() in the queue closure ✅

The invariant is: any Response returned by scheduleAppPrefetchFetch must eventually be passed to snapshotRscResponse or releaseAppPrefetchFetchSlot. Right now that holds, but it is implicit and easy to break — a future caller that fires a queued prefetch and drops the response on an early-return would permanently leak one of the 4 default slots (and, after 4 leaks, wedge all default-priority App prefetching). Consider making this explicit, e.g. a small comment on scheduleAppPrefetchFetch documenting the release contract, or attaching the release to the response in a way that a finally on the fetchPromise itself can guarantee. link.tsx:748.

2. evictPrefetchCacheIfNeeded lost its cheap early-out → O(n) on every prefetch completion (perf)

The old implementation started with if (cache.size < MAX_PREFETCH_CACHE_SIZE) return; — an O(1) guard that skipped the whole sweep in the common case. The new version removed that and unconditionally iterates the entire cache to sweep expired entries before computing byte size (navigation.ts:525-537), and it is now invoked on every snapshot completion (storePrefetchResponse, prefetchRscResponse, seedPrefetchResponseSnapshot). With a 50 MB budget the cache can hold hundreds of small entries, so this turns each prefetch completion into an O(n) full-cache walk plus an O(n) getPrefetchCacheByteSize.

This is a behavior change too: expired entries are now swept even when the cache is far below capacity (the new test sweeps expired entries even when cache is below byte capacity codifies this). That may be desirable, but it means the hot prefetch path does more work than before. Suggest maintaining a running byte total incrementally (add on insert, subtract on delete) so eviction can early-out cheaply, or at minimum gate the expired-sweep behind a cheap check. Per AGENTS.md "avoid repeated hot-path work / hoist... reuse already-computed" guidance.

3. Removed requestIdleCallback deferral for App viewport prefetches (behavior change)

prefetchUrl's scheduler now runs synchronously whenever hasAppNavigationRuntime() is true (link.tsx:439-444), and visible-link prefetches drain from a queueMicrotask-backed queue instead of browser idle. The new test starts App Router viewport prefetches without waiting for browser idle asserts this intentionally, and the concurrency cap in scheduleAppPrefetchFetch is the replacement backpressure. That's a reasonable tradeoff, but it does mean the dynamic import() chain + header hashing + createRscRequestUrl now run eagerly for every visible App link rather than during idle time. Worth confirming this doesn't regress INP on link-dense pages (playground / hackernews examples). Flagging as a deliberate-looking change that isn't called out in the PR description.

4. LIFO drain can starve the first-queued fetch under sustained pressure (minor)

Both drainDefaultAppPrefetchQueue (link.tsx:735) and drainVisibleAppPrefetchQueue (link.tsx:791) use pop() (LIFO). Prioritizing the most-recently-visible link is defensible, but under continuous scrolling that keeps pushing new entries, an early-queued fetch at the bottom of the stack can be starved indefinitely. Next.js's segment cache uses a priority model rather than pure LIFO. Probably fine for the common case; noting it as a fairness edge.

5. Test relies on a fixed 3-turn event-loop drain (test fragility)

flushPrefetchTasks now loops for (let i = 0; i < 3; i++) awaiting setTimeout(0) (link-navigation.test.ts). The comment acknowledges the microtask-queue drain, but a hardcoded turn count is brittle — if the queue depth changes (e.g. shell-first adds another hop) this silently under-drains and the assertions pass/fail nondeterministically. Prefer draining until the queue is observably empty (poll a condition) rather than a magic constant.

Things I checked that are fine

• Navigation-time consumption still matches: findPrefetchCacheEntryForNavigation falls back to normalizeRscCacheLookupUrl which strips the RSC cache-busting param, so the new Next-Router-Segment-Prefetch header on mode === "auto" (which does change the cache-busting hash via app-rsc-cache-busting.ts:172) does not orphan prefetches on click. ✅
• The cache.get(cacheKey) !== entry guards added to the .then/.finally of storePrefetchResponse and prefetchRscResponse correctly handle an entry being evicted while its fetch is still pending. ✅
• Map deletion during the for...of cache sweep and the touchPrefetchCacheEntry delete+set are both safe (touch runs after iteration completes). ✅
• Pending entries count as 0 bytes until snapshotted; concurrent pending fetches are bounded by the new concurrency cap, so this doesn't blow the budget. ✅
• high priority bypasses the queue and never attaches a release symbol; releaseAppPrefetchFetchSlot early-returns for it. Consistent. ✅

[ask-bonk]

Posted the review to PR #2466. Summary of findings:

Reviewed: byte-budget LRU eviction rework, the App-prefetch fetch-slot concurrency queue, and the segment-prefetch header changes. Local tests (prefetch-cache, link-navigation) pass and vp check is clean.

Issues raised (none individually blocking):

1. Implicit slot-release contract (link.tsx:748) — every Response from scheduleAppPrefetchFetch must reach snapshotRscResponse or releaseAppPrefetchFetchSlot, or one of the 4 default slots leaks permanently. Holds today but is fragile; suggest documenting/enforcing it.

2. O(n) on the hot path (navigation.ts:525-537) — the cheap cache.size < MAX early-out was removed, so eviction now walks the whole cache on every prefetch completion (and sweeps expired entries even when far below capacity). Suggest an incrementally-maintained byte total.

3. Removed requestIdleCallback deferral — App viewport prefetches now run eagerly (microtask-backed) instead of during idle; a deliberate-looking tradeoff not mentioned in the PR description. Worth an INP check.

4. LIFO drain fairness — both drain queues use pop(), which can starve the first-queued fetch under sustained scrolling.

5. Test fragility — flushPrefetchTasks hardcodes a 3-turn event-loop drain; prefer polling until the queue empties.

Verified correct: navigation-time consumption still matches despite the new cache-busting header (URL normalization strips the _rsc param), the cache.get(cacheKey) !== entry eviction-race guards, Map-mutation-during-iteration safety, pending-entry byte accounting, and the high-priority queue bypass.

github run

[james-elicx]

/bigbonk review for issues

[ask-bonk]

/bigbonk follow-up review — PR #2466

Focused pass on top of the earlier ask-bonk review. I confirmed the two headline behaviors hold and tests/prefetch-cache.test.ts passes (34/34). Below are findings the first review did not cover, plus one reinforcement. None are hard blockers, but #1 and #2 are worth addressing before merge because they undercut the PR's stated goal (bounding memory/concurrency).

1. Byte-total drifts when cache.set overwrites an existing entry (correctness)

The new byte accounting is only reconciled through deletePrefetchCacheEntry (which subtracts) and adjustPrefetchCacheByteSize (which adds). But two writers call cache.set(cacheKey, entry) directly on a key that may already hold a snapshotted entry without first subtracting the overwritten entry's bytes:

• storePrefetchResponse — navigation.ts:768
• prefetchRscResponse — navigation.ts:918

Compare with seedPrefetchResponseSnapshot (navigation.ts:680-683), which correctly does deletePrefetchCacheEntry(...) on the existing entry first, then set. The store/prefetch paths do not.

When an overwrite happens, the old entry's bytes are never subtracted from trackedPrefetchCacheByteSize, so the tracked total drifts upward permanently. It never self-heals: once trackedPrefetchCache === cache, getPrefetchCacheByteSize returns the sticky running total and never recomputes (navigation.ts:506-518). A leaked total pushes the cache toward premature/over-eager eviction (and in the limit could evict everything on every insert).

Today this is mostly gated — prefetchRscResponse's App-Router callers bail via hasPrefetchCacheEntryForNavigation/prefetched.has(cacheKey) before an exact-key overwrite, and router.prefetch() bails on prefetched.has(cacheKey) (navigation.ts:2053). But storePrefetchResponse is a documented public/back-compat API (next-shims.d.ts:271) with no dedup gate, so an external caller storing the same key twice will drift the total. Since it's cheap to make correct and consistent, delete-first (mirroring seedPrefetchResponseSnapshot) in both writers, or make getPrefetchCacheEntrySize subtract the current cache.get(cacheKey) before set.

2. router.prefetch() bypasses the new concurrency queue (bounding gap)

The PR adds scheduleAppPrefetchFetch with a 4-slot concurrency cap for viewport/link prefetches, but the imperative router.prefetch() path still fires a raw fetch(...) directly (navigation.ts:2058-2068) instead of routing through scheduleAppPrefetchFetch. So:

• imperative prefetches are not bounded by MAX_DEFAULT_APP_PREFETCH_REQUESTS, and
• their responses never occupy a queue slot, so they run fully concurrently regardless of in-flight viewport prefetches.

A page that calls router.prefetch() in a loop (a common pattern for warming a list of routes) remains unbounded — which is exactly the memory/concurrency pressure this PR is trying to contain. Either route this call through scheduleAppPrefetchFetch(() => fetch(...), "low"), or document that imperative prefetch is intentionally exempt.

3. Reinforcing the slot-release contract (builds on prior review #1)

The prior review flagged that every Response from scheduleAppPrefetchFetch must reach snapshotRscResponse or releaseAppPrefetchFetchSlot. Worth adding: the release plumbing now spans two files coupled only by Symbol.for("vinext.appPrefetchFetchSlotRelease") (defined independently in link.tsx:727 and navigation.ts:234). The Symbol.for registry makes that work, but it means the producer (link.tsx) and the two consumers (snapshotRscResponse, releaseAppPrefetchFetchSlot in navigation.ts) can drift apart with no compile-time link. A single exported constant + a short doc comment on scheduleAppPrefetchFetch stating the release contract would make the invariant enforceable rather than convention.

Verified fine

• Navigation consumption still works despite mode === "auto" now sending Next-Router-Segment-Prefetch (which changes the _rsc hash via app-rsc-cache-busting.ts:172): findPrefetchCacheEntryForNavigation falls back to normalizeRscCacheLookupUrl, which strips _rsc (navigation.ts:449-462). ✅
• high-priority path returns fetcher() untagged and releaseAppPrefetchFetchSlot no-ops when the symbol is absent — consistent. ✅
• touchPrefetchCacheEntry's delete+set runs after for...of cache iteration completes, so no mutation-during-iteration hazard. ✅
• cache.get(cacheKey) !== entry guards on the .then/.finally of both async writers correctly handle mid-flight eviction, and snapshotRscResponse's finally still releases the slot in that case. ✅

[ask-bonk]

Posted the review to PR #2466. Summary of findings:

Reviewed: byte-budget LRU rework, the App-prefetch concurrency queue, and the segment-prefetch header changes. tests/prefetch-cache.test.ts passes (34/34).

New issues (beyond the earlier ask-bonk review):

1. Byte-total drift on cache.set overwrite (navigation.ts:768, navigation.ts:918) — storePrefetchResponse and prefetchRscResponse overwrite an existing key without subtracting the old entry's bytes, unlike seedPrefetchResponseSnapshot which correctly deletes-first. The tracked total never self-heals (it's sticky once trackedPrefetchCache === cache), so any drift is permanent and biases eviction. Mostly gated by dedup checks today, but storePrefetchResponse is an ungated public API.

2. router.prefetch() bypasses the new concurrency queue (navigation.ts:2058-2068) — the imperative path fires raw fetch() instead of scheduleAppPrefetchFetch, so it's unbounded and never occupies a queue slot. A router.prefetch() loop stays fully concurrent, undercutting the PR's bounding goal.

3. Slot-release contract fragility (reinforcing prior Virtual module imports break esbuild dependency optimization when vinext is installed from npm #1) — the release plumbing now spans two files coupled only by Symbol.for(...), with the constant defined twice (link.tsx:727, navigation.ts:234) and no compile-time link. Suggested a single exported constant + doc comment.

Verified fine: navigation consumption despite the new cache-busting hash (_rsc normalization), high-priority untagged bypass, touchPrefetchCacheEntry iteration safety, and the mid-flight eviction guards.

github run

[james-elicx]

/bigbonk review for issues

[ask-bonk]

/bigbonk review — PR #2466 (v2)

Reviewed the byte-budget LRU rework, the new scheduleAppPrefetchFetch concurrency queue (now extracted to shims/internal/app-prefetch-fetch-queue.ts), and the segment-prefetch header changes. tests/prefetch-cache.test.ts (36) and tests/link-navigation.test.ts (55) pass locally and vp check is clean.

This v2 addresses the three substantive issues from the earlier reviews:

• Byte-total drift on overwrite (prior Virtual module imports break esbuild dependency optimization when vinext is installed from npm #1) — storePrefetchResponse (navigation.ts:741-744) and prefetchRscResponse (navigation.ts:870-873) now delete-first before cache.set, mirroring seedPrefetchResponseSnapshot. Covered by the new subtracts overwritten prefetch entries before applying the byte LRU test.
• router.prefetch() bypassing the queue (prior Published npm package ships sourcemaps pointing to missing source files #2) — the imperative path now routes through scheduleAppPrefetchFetch(() => fetch(...), "low") (navigation.ts:2059-2069). Covered by limits low-priority router.prefetch requests until queued responses are snapshotted.
• Slot-release contract spanning two files via duplicated Symbol.for (prior fix: exclude vinext from optimizeDeps to prevent virtual module resolution errors #3) — collapsed to a single exported APP_PREFETCH_FETCH_SLOT_RELEASE_KEY + a doc comment stating the release contract (app-prefetch-fetch-queue.ts:3,38-42).

The earlier perf regression (unconditional O(n) sweep) is also resolved: evictPrefetchCacheIfNeeded restores a cheap early-out (totalSize <= MAX returns before any sweep, navigation.ts:545-546) and the byte total is maintained incrementally via adjustPrefetchCacheByteSize. The does not sweep expired entries on under-budget cache writes test codifies the restored behavior.

Verified correct

• Slot release across every response path. snapshotRscResponse releases in a finally (navigation.ts:801-807); the not-ok branch calls releaseAppPrefetchFetchSlot explicitly (navigation.ts:900); the fetcher-reject path releases in the queue closure. The await snapshotRscResponse(response) runs before the cache.get(cacheKey) !== entry guard, so a mid-flight eviction still releases the slot rather than leaking it.
• Shell-first double-schedule doesn't deadlock or leak. The shell and final fetches each take a "low" slot sequentially; await shellEntry?.pending happens before the final scheduleAppPrefetchFetch, so a full queue can't wedge (the shell's slot frees when its body is snapshotted). high priority bypasses the queue and attaches no release symbol, and releaseAppPrefetchFetchSlot no-ops when the symbol is absent — consistent.
• Byte accounting stays consistent. getPrefetchCacheEntrySize (snapshot.byteLength ?? size ?? 0) is symmetric across the adjust(+) on snapshot and adjust(-) on delete; pending entries count as 0 until snapshotted. Map deletion during the for...of cache sweep and the LRU while loop are safe, and the local totalSize mirror stays in sync with the tracked global.
• Navigation-time consumption still matches despite mode === "auto" now sending Next-Router-Segment-Prefetch (which changes the _rsc hash): findPrefetchCacheEntryForNavigation falls back to normalizeRscCacheLookupUrl, which strips the cache-busting param.
• Test-helper fragility fixed. flushPrefetchTasks now polls a caller-supplied until() with a 1s deadline instead of a hardcoded turn count.

Residual notes (non-blocking)

1. Byte tracking is module-global, keyed by cache identity. trackedPrefetchCache/trackedPrefetchCacheByteSize self-heal via a full recompute in getPrefetchCacheByteSize whenever trackedPrefetchCache !== cache. On the client the cache is a stable window.__VINEXT_RSC_PREFETCH_CACHE__, so it never re-recomputes after the first eviction — any accumulated drift would be permanent for the tab's lifetime. The delete-first fixes remove the known drift sources, and Math.max(0, ...) clamps under-count, but there's no periodic reconciliation. Since the value only biases eviction aggressiveness (not correctness of cache contents), this is acceptable; worth a comment noting the invariant that every set/delete on a tracked cache must be paired with an adjust.

2. LIFO drain fairness (reiterating prior review). Both drainDefaultAppPrefetchQueue (shift(), FIFO) and drainVisibleAppPrefetchQueue (pop(), LIFO) — the default queue is now FIFO but the visible queue remains LIFO, so under sustained scrolling an early-queued visible link can still be starved. Fine for the common case; noting the asymmetry.

3. Eager scheduling for App viewport prefetches (prior fix: exclude vinext from optimizeDeps to prevent virtual module resolution errors #3, deliberate). prefetchUrl now runs synchronously when hasAppNavigationRuntime() (link.tsx:441), replacing requestIdleCallback with the microtask-backed queue + 4-slot cap as backpressure. The starts App Router viewport prefetches without waiting for browser idle test makes this intentional. Worth an INP spot-check on link-dense examples (playground / hackernews), but the concurrency cap is a reasonable replacement.

Overall this is a solid, well-tested revision. No blocking issues.

[ask-bonk]

Posted the review to PR #2466.

Summary

Reviewed the v2 revision (byte-budget LRU eviction, the extracted scheduleAppPrefetchFetch concurrency queue, and the segment-prefetch header changes). Both test files pass locally (36 + 55) and vp check is clean.

This v2 resolves all substantive issues from the earlier ask-bonk reviews:

1. Byte-total drift on overwrite — storePrefetchResponse and prefetchRscResponse now delete-first before cache.set, matching seedPrefetchResponseSnapshot. Covered by a new regression test.
2. router.prefetch() bypassing the queue — now routes through scheduleAppPrefetchFetch(..., "low"), covered by a concurrency-cap test.
3. Duplicated Symbol.for slot-release key — collapsed to a single exported constant + a documented release contract in the new app-prefetch-fetch-queue.ts.
4. O(n) hot-path regression — the cheap totalSize <= MAX early-out is restored, with the byte total maintained incrementally.
5. Test fragility — flushPrefetchTasks now polls a condition with a 1s deadline instead of a hardcoded 3-turn drain.

Verified correct: slot release across every response path (including mid-flight eviction), no deadlock/leak in the shell-first double-schedule, symmetric byte accounting, Map-mutation-during-iteration safety, and navigation-time consumption despite the new cache-busting header.

Residual non-blocking notes: the module-global byte tracker never re-reconciles on a stable client cache (drift would be permanent but only biases eviction, not correctness); the visible-prefetch queue is still LIFO while the default queue is FIFO; and eager (non-idle) App viewport prefetching is a deliberate tradeoff worth an INP spot-check.

No blocking issues.

github run

[james-elicx]

/bigbonk review for issues

[ask-bonk]

/bigbonk review — PR #2466

Fresh pass over the byte-budget LRU rework, the extracted scheduleAppPrefetchFetch queue, and the segment-prefetch header changes. tests/prefetch-cache.test.ts passes locally (36/36). The prior ask-bonk reviews already covered byte-total drift (fixed via delete-first), the router.prefetch() queue routing (fixed), the duplicated Symbol.for key (collapsed to one exported constant), the O(n) hot-path early-out (restored), and test fragility (now polls). I confirmed all of those are resolved. Below are findings those reviews did not cover.

1. LRU eviction can throw away in-flight (pending) prefetches without freeing any bytes (correctness/behavior)

evictPrefetchCacheIfNeeded walks the cache in insertion order and deletes the oldest entries until totalSize <= PREFETCH_CACHE_EVICTION_TARGET_SIZE (navigation.ts:560-574). But pending entries count as 0 bytes (getPrefetchCacheEntrySize returns snapshot?.buffer.byteLength ?? size ?? 0, and pending entries have neither), and the loop has no guard to skip them.

Concrete scenario — cache in insertion order [P (pending, 0 B), S (60 MB, just snapshotted)], target 45 MB:

1. totalSize = 60 MB > 45 MB → oldest is P; delete it; totalSize -= 0 → still 60 MB.
2. 60 MB > 45 MB → oldest is now S; delete it; totalSize -= 60 MB → 0.
3. Exit.

Two problems:

• P, a legitimate in-flight prefetch, is orphaned. When its fetch resolves, the cache.get(cacheKey) !== entry guard (navigation.ts:753, navigation.ts:888) discards the result, so the fetch (and its queue slot, until the body is read) was wasted.
• S, the entry whose completion just triggered eviction, is itself evicted, because it's the oldest sized entry. The freshly prefetched payload is thrown away immediately after being stored.

This ordering is realistic: with the 4-slot concurrency cap and variable response latencies, an early-queued slow prefetch stays pending at the front of the map while newer, faster ones complete and trigger eviction. Under sustained memory pressure the eviction preferentially deletes pending entries (0 B) that don't reduce memory, then over-shoots into freshly-completed entries.

Next.js's segment cache eviction operates on entries that have a measurable size and treats in-progress fetches separately rather than LRU-evicting them by pure insertion order. Suggest skipping pending (0-byte) entries in the LRU loop (if (getPrefetchCacheEntrySize(entry) === 0) continue; or explicitly if (entry.pending !== undefined) continue;), so eviction only reclaims entries that actually hold buffered bytes.

2. Server path returns a throwaway cache Map, so byte tracking is meaningless there (robustness)

getPrefetchCache() returns a new Map on every call when isServer (navigation.ts:308). If any byte-tracking path runs server-side, getPrefetchCacheByteSize would set trackedPrefetchCache to a per-call throwaway map and immediately recompute (total 0) on the next call, and adjustPrefetchCacheByteSize would no-op against a different map. This is currently harmless because these paths are client-only (app-prefetch-fetch-queue.ts is "use client" and eviction is only triggered from client snapshot completions), but the module-global tracker (trackedPrefetchCache/trackedPrefetchCacheByteSize) is shared across environments with no isServer guard. Worth an explicit early-return or a comment documenting that eviction/byte-tracking is client-only, so a future server-side caller of storePrefetchResponse/seedPrefetchResponseSnapshot doesn't silently corrupt the tracker.

3. Module-global byte tracker never re-reconciles on a stable client cache (reinforcing v2 residual note)

Reiterating the v2 residual note with the concrete invariant: on a real client the cache is a stable window.__VINEXT_RSC_PREFETCH_CACHE__, so after the first getPrefetchCacheByteSize call trackedPrefetchCache === cache holds forever and the full recompute never runs again. That means every cache.set/cache.delete on the tracked map must be paired with an adjustPrefetchCacheByteSize (or go through deletePrefetchCacheEntry) or the total drifts permanently for the tab lifetime. The delete-first fixes closed the known drift sources, but this is an implicit, unenforced invariant. A short comment on trackedPrefetchCacheByteSize stating "every mutation of a tracked cache must adjust this" would make the coupling discoverable. (The Math.max(0, ...) clamp only guards under-count, not over-count.)

Verified fine

• Slot release across every response path, including mid-flight eviction: snapshotRscResponse runs its arrayBuffer() read (and finally release) before the cache.get(cacheKey) !== entry guard in both storePrefetchResponse and prefetchRscResponse, so an evicted entry still frees its slot. The not-ok branch releases explicitly (navigation.ts:900); the reject path releases in the queue closure.
• touchPrefetchCacheEntry's delete+set runs only after findPrefetchCacheEntryForNavigation finishes its for...of iteration, so no mutation-during-iteration hazard.
• Overwrite delete-first in storePrefetchResponse (navigation.ts:741-744) and prefetchRscResponse (navigation.ts:867) matches seedPrefetchResponseSnapshot; the local totalSize mirror in the eviction loop stays in sync with the tracked global (both use getPrefetchCacheEntrySize).
• Link cleanup now sets instance.isVisible = false and the drain skips !isVisible || routerMode !== "app", so unmounted/queued links don't fire stale prefetches.
• Navigation-time consumption still matches despite mode === "auto" now sending Next-Router-Segment-Prefetch (which changes the _rsc hash): findPrefetchCacheEntryForNavigation falls back to normalizeRscCacheLookupUrl which strips the cache-busting param.

Issue #1 is the one I'd want addressed before merge — the others are hardening/documentation. Nice, well-tested revision overall.

[ask-bonk]

Posted the review to PR #2466.

Summary

Reviewed the byte-budget LRU rework, the extracted scheduleAppPrefetchFetch concurrency queue, and the segment-prefetch header changes. tests/prefetch-cache.test.ts passes locally (36/36). Confirmed all substantive issues from the prior ask-bonk reviews are resolved (byte-total drift, router.prefetch() queue routing, duplicated Symbol.for key, O(n) hot-path early-out, test fragility).

New findings not covered by earlier reviews:

1. LRU can evict in-flight (pending) prefetches without freeing bytes (navigation.ts:560-574) — the eviction loop deletes oldest-by-insertion entries, but pending entries count as 0 bytes and there's no guard to skip them. Under memory pressure it can orphan legitimate in-flight prefetches (0 B, no progress) and then over-shoot into the freshly-completed entry that just triggered eviction. Realistic given the 4-slot cap + variable latency. Suggested skipping 0-byte/pending entries in the LRU loop. This is the one worth addressing before merge.

2. Server path returns a throwaway Map (navigation.ts:308) — the module-global byte tracker has no isServer guard; harmless today since these paths are client-only, but a future server-side caller could silently corrupt it. Suggested an explicit guard/comment.

3. Module-global tracker never re-reconciles on a stable client cache (reinforcing the v2 residual note) — on a real client the cache map is stable for the tab lifetime, so the full recompute never runs again and every mutation must be paired with an adjust or the total drifts permanently. Suggested documenting the invariant.

Verified fine: slot release across every response path including mid-flight eviction, touchPrefetchCacheEntry iteration safety, overwrite delete-first correctness and totalSize mirror sync, link-cleanup isVisible handling, and navigation-time consumption despite the new cache-busting header.

github run

[james-elicx]

/bigbonk review for issues

[ask-bonk]

/bigbonk review — PR #2466

Fresh pass over the current HEAD (c2d194e) covering the byte-budget LRU rework, the extracted scheduleAppPrefetchFetch queue, the router.prefetch() queue routing, and the segment-prefetch header changes. tests/prefetch-cache.test.ts (37), tests/link-navigation.test.ts (55), and tests/link.test.ts (121) all pass locally.

All substantive findings from the earlier ask-bonk reviews are resolved in the current tree, including the last review's headline concern (#1, LRU evicting pending 0-byte prefetches):

• Pending-eviction fix — evictPrefetchCacheIfNeeded now skips pending 0-byte entries by touching them to the back and counting inspectedEntries (navigation.ts:566-571), so the loop can walk every entry, skip in-flight ones, and still terminate. Covered by skips pending prefetches when byte LRU cleanup needs to free memory.
• Byte-total drift on overwrite — storePrefetchResponse (navigation.ts:750-753) and prefetchRscResponse (navigation.ts:879-882) delete-first before cache.set, matching seedPrefetchResponseSnapshot.
• router.prefetch() bounding — now routes through scheduleAppPrefetchFetch(() => fetch(...), "low") (navigation.ts:2068-2078), covered by limits low-priority router.prefetch requests until queued responses are snapshotted.
• Duplicated Symbol.for slot-release key — collapsed to one constant + a documented release contract in app-prefetch-fetch-queue.ts:3,38-42.
• O(n) hot-path — the cheap totalSize <= MAX early-out is restored (navigation.ts:546) with the byte total maintained incrementally.
• Test fragility — flushPrefetchTasks now settles dynamic imports and polls a caller-supplied until() with a 1s deadline.

Independently verified this pass

• Segment-prefetch header is a pure cache-key change, not a content change. NEXT_ROUTER_SEGMENT_PREFETCH_HEADER / NEXT_ROUTER_PREFETCH_HEADER on mode === "auto" only feed createCacheBustingInput (app-rsc-cache-busting.ts:171-172); neither header alters server rendering (only renderMode/PREFETCH_LOADING_SHELL does). So auto-mode prefetches still fetch the full navigation payload, and navigation-time consumption still matches because findPrefetchCacheEntryForNavigation falls back to normalizeRscCacheLookupUrl, which strips the cache-busting param (navigation.ts:452-461).
• Eviction loop termination is sound. Traced [P(0B), S1, S2], [P1, P2, S], and single-over-budget-entry cases — pending entries are always skipped, sized entries reset inspectedEntries, and since pending entries contribute 0 bytes the total can never stay above target while only pending entries remain, so the inspectedEntries < cache.size guard always terminates.
• Slot release across every response path, including mid-flight eviction: snapshotRscResponse's arrayBuffer() read (and finally release) runs before the cache.get(cacheKey) !== entry guard in both async writers; the not-ok branch releases explicitly (navigation.ts:909) with no double-release (releaseAppPrefetchFetchSlot nulls the symbol); the reject path releases in the queue closure.
• Shell-first double-schedule can't wedge the queue. The shell slot is freed when its body is snapshotted, and await shellEntry?.pending precedes the final scheduleAppPrefetchFetch, so the two "low" slots are taken sequentially, not held simultaneously.
• high priority bypasses the queue and attaches no release symbol; releaseAppPrefetchFetchSlot no-ops when the symbol is absent — consistent.
• Link cleanup sets instance.isVisible = false and both drains skip !isVisible || routerMode !== "app", so unmounted/queued links don't fire stale prefetches.

Residual non-blocking notes

1. Module-global byte tracker never re-reconciles on a stable client cache. On a real client the cache is a stable window.__VINEXT_RSC_PREFETCH_CACHE__, so after the first getPrefetchCacheByteSize call trackedPrefetchCache === cache holds for the tab's lifetime and the full recompute never runs again. Every set/delete on the tracked cache must be paired with an adjustPrefetchCacheByteSize (or go through deletePrefetchCacheEntry). The delete-first fixes closed the known drift sources and Math.max(0, ...) clamps under-count, but this is an implicit, unenforced invariant that only biases eviction aggressiveness (not cache correctness). A one-line comment on trackedPrefetchCacheByteSize stating the invariant would make the coupling discoverable.
2. drainVisibleAppPrefetchQueue drains the whole queue in a single microtask, so its pop() (LIFO) only reorders synchronous prefetchUrl calls within one batch — the real backpressure is the 4-slot cap in scheduleAppPrefetchFetch. The earlier "LIFO starvation" note is therefore mostly moot for the visible queue; the ordering asymmetry vs the FIFO default queue is cosmetic.
3. Eager (non-idle) App viewport prefetching (link.tsx:441) replaces requestIdleCallback with the microtask queue + 4-slot cap. Deliberate and now test-asserted (starts App Router viewport prefetches without waiting for browser idle); worth an INP spot-check on link-dense examples (playground / hackernews), but the cap is a reasonable replacement.

No blocking issues. This is a solid, well-tested revision.

[ask-bonk]

Posted the review to PR #2466.

Summary

Reviewed the current HEAD (c2d194e) — byte-budget LRU eviction, the extracted scheduleAppPrefetchFetch concurrency queue, router.prefetch() queue routing, and the segment-prefetch header changes. Tests pass locally (prefetch-cache 37, link-navigation 55, link 121).

All substantive issues from the prior ask-bonk reviews are resolved in the current tree, including the last review's headline concern (LRU evicting in-flight pending prefetches) — the eviction loop now skips pending 0-byte entries via touchPrefetchCacheEntry + inspectedEntries and is covered by a regression test.

Independently verified this pass:

• The new Next-Router-Segment-Prefetch / Next-Router-Prefetch headers on mode === "auto" are a pure cache-key change — they only feed the cache-busting hash, not server rendering, so auto prefetches still fetch full payloads and navigation consumption matches via _rsc normalization.
• Eviction loop termination traced through multiple pending/sized orderings — always terminates and never orphans sized entries prematurely.
• Slot release holds across every response path including mid-flight eviction; no double-release; shell-first double-schedule can't wedge the queue.

Residual non-blocking notes: the module-global byte tracker never re-reconciles on a stable client cache (an implicit invariant worth a comment); the visible-prefetch queue's LIFO ordering is cosmetic since it drains fully per microtask; and eager non-idle viewport prefetching is a deliberate tradeoff worth an INP spot-check.

No blocking issues.

github run

[james-elicx]

Backlog mapping for GitHub Actions run 28478866791 / report job 84413308650:

This PR is the existing fix for the two non-cacheComponents segment-cache/memory-pressure rows, so I am skipping duplicate branch work.

Original failed rows from the downloaded test-results-1 artifact:

• test/e2e/app-dir/segment-cache/memory-pressure/segment-cache-memory-pressure.test.ts > segment cache memory pressure > evicts least recently used prefetch data once cache size exceeds limit
  • failure: Timed out waiting for a request to be initiated. at upstream line 30, the first tab switch that should kick off a blocked prefetch for Page 0.
  • upstream assertion path: tab 1 starts Page 0 prefetch, tab 2 overflows the cache with Page 1, tab 1 must initiate a new Page 0 prefetch after LRU eviction, and tab 2 must not refetch recently accessed Page 0/Page 1 entries.
• test/e2e/app-dir/segment-cache/memory-pressure/segment-cache-memory-pressure.test.ts > segment cache memory pressure > does not leak memory when repeatedly triggering prefetches
  • failure: Jest Exceeded timeout of 120000 ms at upstream line 67.
  • upstream assertion path: repeatedly mount/unmount tab 2 prefetch links, wait for prefetches to settle each cycle, collect heap samples, and require the post-warmup regression slope to stay below 0.1 MB/cycle.

Validation already done on this PR:

• focused vinext coverage in tests/link-navigation.test.ts and tests/prefetch-cache.test.ts
• targeted upstream wrapper: REPO="$(pwd)" NEXTJS_DIR="/Users/jamesanderson/Developer/vinext/.nextjs-ref" NEXT_TEST_CONCURRENCY=1 ./scripts/run-targeted-nextjs-e2e.sh test/e2e/app-dir/segment-cache/memory-pressure/segment-cache-memory-pressure.test.ts
• /bigbonk review for issues loop completed with no blockers before merge
• PR checks were green, and the merge commit bc1dfce00f52ce4c6fdda2392a16fb31e5d11dbd is contained in origin/main

[ask-bonk]

fatal: couldn't find remote ref codex/fix-segment-cache-memory-pressure-v2-28478866791

github run

[ask-bonk]

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

View workflow run · To retry, trigger Bonk again.