diff --git a/src/head-sync.ts b/src/head-sync.ts index 392115c..0439fb6 100644 --- a/src/head-sync.ts +++ b/src/head-sync.ts @@ -198,7 +198,13 @@ export async function syncHeadStyles(newDoc: Document): Promise { if (loadPromises.length > 0) { dlog('head', `stylesheets: +${loadPromises.length} new`); - const timeout = new Promise((resolve) => setTimeout(resolve, 2000)); + const timeout = new Promise((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]); } } diff --git a/src/prefetch.ts b/src/prefetch.ts index 25c3ab2..c0d4596 100644 --- a/src/prefetch.ts +++ b/src/prefetch.ts @@ -19,6 +19,10 @@ const CACHE_MAX_ENTRIES = 32; const cache = new Map(); const inFlight = new Map>(); +// 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(); 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 | 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 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 } })().finally(() => { inFlight.delete(key); + started.delete(key); }); inFlight.set(key, promise); @@ -382,9 +401,11 @@ 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 @@ -392,6 +413,23 @@ export function _resetPrefetchForTests(): void { // /cart, /account, /checkout are skipped. const warmedFrom = new Set(); +// 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); } diff --git a/test/dom.ts b/test/dom.ts index 2ff7615..6e30507 100644 --- a/test/dom.ts +++ b/test/dom.ts @@ -81,6 +81,21 @@ export function setupDom(initialTemplate = 'index', initialBody = '

Home

(globalThis as Record).CSS = { escape: (s: string) => s.replace(/[^a-zA-Z0-9_-]/g, '\\$&') }; } + // jsdom never fires load or error on an injected , 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; @@ -88,6 +103,7 @@ export function setupDom(initialTemplate = 'index', initialBody = '

Home

(w.history as unknown as { scrollRestoration: ScrollRestoration }).scrollRestoration = 'auto'; const reset = () => { + styleLoads.disconnect(); dom.window.close(); }; diff --git a/test/smoke.test.ts b/test/smoke.test.ts index 0933bc0..9cfee21 100644 --- a/test/smoke.test.ts +++ b/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).fetch = () => + new Promise((resolve) => { + release.push(() => + resolve(new Response(makePageHtml('product', '

x

'), { + 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 += `Cart`; runtime.initRuntime();