A click should never wait on a prefetch that never started

src/head-sync.ts

@@ -198,7 +198,13 @@ export async function syncHeadStyles(newDoc: Document): Promise<void> {
 
   if (loadPromises.length > 0) {
     dlog('head', `stylesheets: +${loadPromises.length} new`);
-    const timeout = new Promise<void>((resolve) => setTimeout(resolve, 2000));
+    const timeout = new Promise<void>((resolve) => {
+      const timer = setTimeout(resolve, 2000);
+      // Same reason as the script-load cap above: under Node this Timeout
+      // would otherwise hold the event loop open for the full 2s after the
+      // document is gone.
+      (timer as unknown as { unref?: () => void }).unref?.();
+    });
     await Promise.race([Promise.all(loadPromises), timeout]);
   }
 }

src/prefetch.ts

@@ -19,6 +19,10 @@ const CACHE_MAX_ENTRIES = 32;
 
 const cache = new Map<string, CacheEntry>();
 const inFlight = new Map<string, Promise<void>>();
+// Keys whose fetch has actually begun. A warm sits in `inFlight` from the
+// moment it is requested, but does not reach the network until it acquires a
+// concurrency slot — see peekInFlight for why the difference matters.
+const started = new Set<string>();
 
 function trimCache(): void {
   while (cache.size > CACHE_MAX_ENTRIES) {
@@ -114,12 +118,25 @@ export function getCachedHtml(url: string): string | null {
   return entry.html;
 }
 
-// Sync probe — returns the in-flight prefetch promise for `url` if one is
-// active, else null. Used by runtime.navigate() to dedup against an in-flight
-// hover/touch warmup: if the user clicks a link mid-warmup, we await the
-// prefetch instead of firing a duplicate fetch.
+// Sync probe — returns the prefetch promise for `url` only if its fetch is
+// already on the wire, else null. Used by runtime.navigate() to dedup against
+// an in-flight hover/touch warmup: if the user clicks a link mid-warmup,
+// awaiting it costs ~the same wall time and saves the duplicate round-trip.
+//
+// The `started` gate is load-bearing. A warm is in `inFlight` from the moment
+// it is requested, but MAX_CONCURRENT_PREFETCH may leave it queued behind two
+// others without a single byte sent. Returning it then makes a real navigation
+// wait on work that has not begun — the exact opposite of the guarantee the
+// limiter above claims ("real navigation always bypasses this queue"), and on a
+// slow connection it is a user-visible stall bounded only by the nav timeout.
+//
+// Bypassing costs at most one speculative duplicate: the queued warm still runs
+// later for a page the buyer has already navigated to. That is strictly the
+// better trade — wasted bandwidth on a background request beats a stalled click.
 export function peekInFlight(url: string): Promise<void> | null {
-  return inFlight.get(toPathKey(url)) ?? null;
+  const key = toPathKey(url);
+  if (!started.has(key)) return null;
+  return inFlight.get(key) ?? null;
 }
 
 export function invalidateCache(predicate?: (url: string) => boolean): void {
@@ -194,6 +211,7 @@ export function prefetchPage(url: string, { force = false } = {}): Promise<void>
 
   const promise = (async () => {
     await acquirePrefetchSlot();
+    started.add(key);
     try {
       // Checked again after waiting for a slot: a real navigation may have
       // fetched this page while we queued, which makes the warm pure waste.
@@ -242,6 +260,7 @@ export function prefetchPage(url: string, { force = false } = {}): Promise<void>
     }
   })().finally(() => {
     inFlight.delete(key);
+    started.delete(key);
   });
 
   inFlight.set(key, promise);
@@ -382,16 +401,35 @@ export function _resetPrefetchForTests(): void {
   clearHoverTimeout();
   cache.clear();
   inFlight.clear();
+  started.clear();
   warmedFrom.clear();
   activePrefetches = 0;
   prefetchQueue.length = 0;
+  cancelPendingWarmups();
 }
 
 // Eager nav-link warmup. Runs at requestIdleCallback time on initial load
 // and after PJAX swaps onto index/page templates. Same-page links and
 // /cart, /account, /checkout are skipped.
 const warmedFrom = new Set<string>();
 
+// A scheduled warmup outlives the call that scheduled it, and nothing held a
+// handle to cancel it. Harmless in a browser — the callback runs against the
+// same document, and `warmedFrom` stops it repeating. Not harmless in the
+// suite: each test tears its document down and builds a fresh one, so a warmup
+// scheduled by an earlier test fired inside a later one and warmed the new
+// document's nav links. That is what made `prefetch never runs more than two
+// requests at once` fail on node 22 and pass on 24 — a seventh fetch, arriving
+// against a stub that had accounted for six, leaving one in flight at assert
+// time. The bug was never in the limiter; the timing just decided which test
+// caught the stray.
+const pendingWarmups = new Set<() => void>();
+
+function cancelPendingWarmups(): void {
+  for (const cancel of pendingWarmups) cancel();
+  pendingWarmups.clear();
+}
+
 export interface NavWarmupOptions {
   /** Selector for the nav container. Defaults to '#header-group, header'. */
   selector?: string;
@@ -406,8 +444,8 @@ export function warmupNavLinks(options: NavWarmupOptions = {}): void {
   if (warmedFrom.has(window.location.pathname)) return;
   warmedFrom.add(window.location.pathname);
 
-  const idle = window.requestIdleCallback ?? ((cb: () => void) => setTimeout(cb, 200));
-  idle(() => {
+  const run = () => {
+    pendingWarmups.delete(cancel);
     const sel = options.selector ?? '#header-group, header';
     const header = document.querySelector(sel);
     if (!header) {
@@ -429,5 +467,17 @@ export function warmupNavLinks(options: NavWarmupOptions = {}): void {
       warmed++;
     });
     if (warmed) dlog('prefetch', `nav-link warmup: ${warmed} links`);
-  });
+  };
+
+  // jsdom ships no requestIdleCallback, so the timeout path is the one the
+  // suite exercises — and the one that needed a handle.
+  let cancel: () => void;
+  if (typeof window.requestIdleCallback === 'function') {
+    const handle = window.requestIdleCallback(run);
+    cancel = () => window.cancelIdleCallback?.(handle);
+  } else {
+    const handle = setTimeout(run, 200);
+    cancel = () => clearTimeout(handle);
+  }
+  pendingWarmups.add(cancel);
 }

test/dom.ts

@@ -81,13 +81,29 @@ export function setupDom(initialTemplate = 'index', initialBody = '<h1>Home</h1>
     (globalThis as Record<string, unknown>).CSS = { escape: (s: string) => s.replace(/[^a-zA-Z0-9_-]/g, '\\$&') };
   }
 
+  // jsdom never fires load or error on an injected <link rel="stylesheet">, so
+  // syncHeadStyles waits out its full 2s cap on every test that syncs one — 4s
+  // of suite time across two tests. A real browser fires load, so this restores
+  // browser behaviour rather than shortening a product timeout for the tests.
+  const styleLoads = new w.MutationObserver((records) => {
+    for (const record of records) {
+      record.addedNodes.forEach((node) => {
+        const el = node as HTMLLinkElement;
+        if (el.tagName !== 'LINK' || el.getAttribute('rel') !== 'stylesheet') return;
+        setTimeout(() => el.dispatchEvent(new w.Event('load')), 0);
+      });
+    }
+  });
+  styleLoads.observe(w.document.head, { childList: true });
+
   // jsdom doesn't implement scrollTo; stub to avoid Not implemented warnings.
   w.scrollTo = (() => {}) as typeof w.scrollTo;
 
   // Reset history.scrollRestoration setter — jsdom allows it.
   (w.history as unknown as { scrollRestoration: ScrollRestoration }).scrollRestoration = 'auto';
 
   const reset = () => {
+    styleLoads.disconnect();
     dom.window.close();
   };
 

test/smoke.test.ts

@@ -742,6 +742,44 @@ test('prefetch never runs more than two requests at once', async () => {
   assert.equal(inFlight, 0, 'no slot left held');
 });
 
+test('peekInFlight ignores a warm that is still queued for a slot', async () => {
+  // The stall that motivated this: two warms hold both slots of
+  // MAX_CONCURRENT_PREFETCH and never resolve, so a third is queued with no
+  // bytes on the wire. navigate() must not await it — it has not started.
+  const release: Array<() => void> = [];
+  (globalThis as Record<string, unknown>).fetch = () =>
+    new Promise<Response>((resolve) => {
+      release.push(() =>
+        resolve(new Response(makePageHtml('product', '<h1>x</h1>'), {
+          status: 200,
+          headers: { 'Content-Type': 'text/html' },
+        })),
+      );
+    });
+
+  const prefetch = await import('../src/prefetch.ts');
+  void prefetch.prefetchPage('/a');
+  void prefetch.prefetchPage('/b');
+  void prefetch.prefetchPage('/queued');
+  // Let the two that won slots reach fetch().
+  await new Promise((r) => setTimeout(r, 0));
+
+  assert.equal(release.length, 2, 'exactly two warms are on the wire');
+  assert.notEqual(prefetch.peekInFlight('/a'), null, 'a started warm is still offered');
+  assert.equal(
+    prefetch.peekInFlight('/queued'),
+    null,
+    'a queued warm is withheld — awaiting it would stall the navigation',
+  );
+
+  // Once a slot frees and the queued warm starts, it becomes dedup-worthy.
+  release[0]();
+  await new Promise((r) => setTimeout(r, 0));
+  assert.notEqual(prefetch.peekInFlight('/queued'), null, 'offered once it starts');
+
+  release.forEach((fn) => fn());
+});
+
 test('a redirect log fires only when the page actually moved', async () => {
   // `response.url` never carries a fragment, so comparing before restoring it
   // made every #hash navigation report as a redirect.
@@ -1070,6 +1108,29 @@ test('nav-link warmup excludes SHOPIFY_RESERVED routes (incl. /customer_authenti
   assert.ok(warmedUrls.some((u) => u.includes('/collections/all')), 'normal links still warmed');
 });
 
+test('a scheduled nav-link warmup does not survive a reset', async () => {
+  // The leak this pins: warmup schedules a callback 200ms out and nothing held
+  // the handle, so a warmup scheduled by one test fired inside a later one and
+  // spent a fetch on the fresh document's nav links. It surfaced as the
+  // concurrency test seeing a seventh request it had not accounted for —
+  // intermittently, and only on the node version whose timing lined the two up.
+  // Asserting on fetch traffic after a reset is the honest shape: the stray
+  // request is the damage, wherever it lands.
+  const prefetchModule = await import('../src/prefetch.ts');
+
+  prefetchModule.warmupNavLinks();
+  prefetchModule._resetPrefetchForTests();
+
+  const before = fetchCalls.length;
+  await new Promise((r) => setTimeout(r, 250));
+
+  assert.equal(
+    fetchCalls.length,
+    before,
+    'a warmup cancelled by reset must not reach the wire',
+  );
+});
+
 test('/cart IS intercepted (regular themed page)', async () => {
   document.body.innerHTML += `<a id="cart" href="/cart">Cart</a>`;
   runtime.initRuntime();