From dd9f2ebba539eccbec35ee0076b8c44dee5bfcb0 Mon Sep 17 00:00:00 2001 From: Connor Tsui Date: Tue, 8 Sep 2026 11:43:42 -0400 Subject: [PATCH] Keep bundle failures shared until an explicit retry Signed-off-by: Connor Tsui --- web/components/Chart.lazy-hydration.test.tsx | 66 ++++++++- web/components/Chart.tsx | 34 ++++- web/lib/chart-store.test.ts | 44 ++++++ web/lib/chart-store.ts | 136 +++++++++++-------- 4 files changed, 221 insertions(+), 59 deletions(-) diff --git a/web/components/Chart.lazy-hydration.test.tsx b/web/components/Chart.lazy-hydration.test.tsx index 2177184..7204dde 100644 --- a/web/components/Chart.lazy-hydration.test.tsx +++ b/web/components/Chart.lazy-hydration.test.tsx @@ -8,6 +8,7 @@ import { createRoot, type Root } from 'react-dom/client'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { Chart } from '@/components/Chart'; +import { FETCH_TIMEOUT_MS } from '@/lib/chart-format'; import { bundleQueue, hydrationQueue, resetPayloadCache } from '@/lib/chart-store'; vi.mock('@/lib/chart-js', () => ({ @@ -84,10 +85,14 @@ describe('PR-5.0.95 landing-page lazy hydration', () => { beforeEach(() => { (globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true; fetchCalls = []; + resetPayloadCache(); MockIO.instances = []; vi.stubGlobal('IntersectionObserver', MockIO); vi.stubGlobal('fetch', (url: string | URL) => { fetchCalls.push(String(url)); + if (String(url).includes('/api/group/')) { + return Promise.resolve({ ok: false, status: 404 } as Response); + } return Promise.resolve(jsonResponse(windowedPayload(3572))); }); container = document.createElement('div'); @@ -304,6 +309,9 @@ describe('PR-5.0.95 landing-page lazy hydration', () => { const signals: AbortSignal[] = []; vi.stubGlobal('fetch', (url: string | URL, init?: { signal?: AbortSignal }) => { fetchCalls.push(String(url)); + if (String(url).includes('/api/group/')) { + return Promise.resolve({ ok: false, status: 404 } as Response); + } if (init?.signal) { signals.push(init.signal); } @@ -441,6 +449,60 @@ describe('PR-5.0.97 group-bundle hydration', () => { expect(chartFetchCount()).toBe(0); }); + it.each(['500', '503', 'network', 'timeout', 'null response'])( + 'a bundle %s offers a shared retry without per-chart fanout or passive retries', + async (failure) => { + vi.useFakeTimers(); + let attempts = 0; + vi.stubGlobal('fetch', (url: string | URL, init?: RequestInit) => { + fetchCalls.push(String(url)); + attempts += 1; + if (attempts > 1) { + return Promise.resolve(bundleResponse(['s0', 's1', 's2'])); + } + if (failure === 'null response') { + return Promise.resolve(jsonResponse(null)); + } + if (failure === 'network') { + return Promise.reject(new TypeError('network unavailable')); + } + if (failure === 'timeout') { + return new Promise((_resolve, reject) => { + init?.signal?.addEventListener('abort', () => reject(init.signal?.reason)); + }); + } + return Promise.resolve({ ok: false, status: Number(failure) } as Response); + }); + await renderGroup(3); + await act(async () => { + MockIO.instances[0].fire(); + MockIO.instances[1].fire(); + await vi.advanceTimersByTimeAsync(FETCH_TIMEOUT_MS); + }); + expect(container.querySelectorAll('[data-role="fetch-retry"]')).toHaveLength(2); + expect(bundleFetchCount()).toBe(1); + expect(chartFetchCount()).toBe(0); + + // A later card sees the same failure without starting another request. + await act(async () => { + MockIO.instances[2].fire(); + await vi.advanceTimersByTimeAsync(FETCH_TIMEOUT_MS * 2); + }); + expect(container.querySelectorAll('[data-role="fetch-retry"]')).toHaveLength(3); + expect(bundleFetchCount()).toBe(1); + const retries = container.querySelectorAll('[data-role="fetch-retry"]'); + await act(async () => { + retries[0].click(); + retries[1].click(); + }); + expect(bundleFetchCount()).toBe(2); + expect(chartFetchCount()).toBe(0); + expect(container.querySelectorAll('.chart-error')).toHaveLength(0); + const chips = container.querySelectorAll('[data-role="window-chip"]'); + expect([...chips].every((chip) => chip.dataset.state === 'windowed')).toBe(true); + }, + ); + it('closing the group aborts the in-flight bundle (its fetch signal aborts)', async () => { const signals: AbortSignal[] = []; vi.stubGlobal('fetch', (url: string | URL, init?: { signal?: AbortSignal }) => { @@ -633,8 +695,8 @@ describe('PR-5.0.97 group-bundle hydration', () => { }); it('reopen after a bundle 404 re-issues the group bundle fetch', async () => { - // A 404 leaves `completedBundles` unset; `abortGroupBundle` (on close) clears - // `attemptedBundles`, so a reopen must re-attempt the bundle rather than + // Closing clears the settled unavailable outcome, so a reopen must + // re-attempt the bundle rather than // short-circuit. This pins that re-attempt behavior. vi.stubGlobal('fetch', (url: string | URL) => { fetchCalls.push(String(url)); diff --git a/web/components/Chart.tsx b/web/components/Chart.tsx index 3161a02..6dee2bc 100644 --- a/web/components/Chart.tsx +++ b/web/components/Chart.tsx @@ -66,8 +66,10 @@ import { hydrationQueue, noteChartRecentData, noteGroupSeries, + retryGroupBundle, subscribeGlobalFilter, subscribeGroup, + subscribeGroupBundleRetry, type QueueEntry, } from '@/lib/chart-store'; import type { ChartResponse } from '@/lib/queries'; @@ -409,6 +411,8 @@ class ChartController { private readonly aborter = new AbortController(); /** Failed Chart.js dynamic-import attempts; bounds the error-dismiss retry. */ private loadAttempts = 0; + private bundleFailed = false; + private readonly unsubscribeBundleRetry?: () => void; constructor( private readonly slug: string, @@ -440,6 +444,15 @@ class ChartController { wheelAttached: false, disposed: false, }; + if (groupSlug) { + this.unsubscribeBundleRetry = subscribeGroupBundleRetry(groupSlug, () => { + if (this.bundleFailed && this.groupIsOpen() && !this.state.disposed) { + this.bundleFailed = false; + this.cb.setError(null); + this.onGroupOpen(0); + } + }); + } } /** Seed the permalink page's server-fetched payload before any fetch runs. */ @@ -499,20 +512,22 @@ class ChartController { } // On the landing page (a group slug is present), drive one bundle fetch per // group and hydrate from it. Only fall through to the per-chart fetch when - // the bundle is unavailable (404 / failed / this slug missing). + // the bundle is unavailable (404) or this slug is missing. if (this.groupSlug) { if (showLoading) { + this.cb.setError(null); this.cb.setLoading(true); this.cb.setRetryable(false); } const groupSlug = this.groupSlug; - return ensureGroupBundle(groupSlug, priority).then(() => { + this.bundleFailed = false; + return ensureGroupBundle(groupSlug, priority).then((result) => { // The group can close while this card awaits the in-flight bundle. The // close runs `abortInFlightFetches` + `abortGroupBundle` already, so a // per-chart fallback issued now would never be aborted and would defeat // the "closing a group frees server capacity" contract. Bail when the // group is no longer open. - if (state.disposed || state.payload || !this.groupIsOpen()) { + if (state.disposed || state.payload || !this.groupIsOpen() || result.status === 'aborted') { return; } const fromBundle = getCachedPayload(this.slug); @@ -520,6 +535,13 @@ class ChartController { this.seedFromCachedPayload(fromBundle); return; } + if (result.status === 'failed') { + this.bundleFailed = true; + this.cb.setLoading(false); + this.cb.setError('failed to load group charts'); + this.cb.setRetryable(true); + return; + } // Bundle did not cover this chart: fall back to the per-chart fetch. return this.fetchInitialPayloadDirect(priority, showLoading); }); @@ -546,6 +568,8 @@ class ChartController { } this.syncWindowChip(); this.cb.setLoading(false); + this.cb.setError(null); + this.cb.setRetryable(false); if (this.groupSlug) { noteGroupSeries(this.groupSlug, normalized.series_meta); } @@ -685,6 +709,9 @@ class ChartController { if (this.state.disposed || this.state.payload) { return; } + if (this.groupSlug && this.bundleFailed && retryGroupBundle(this.groupSlug)) { + return; + } this.cb.setError(null); this.cb.setRetryable(false); void this.ensureInitialPayload(0, true).then(() => { @@ -1714,6 +1741,7 @@ class ChartController { * the mount effect constructs a fresh controller for the next mount. */ destroy(): void { this.state.disposed = true; + this.unsubscribeBundleRetry?.(); this.aborter.abort(new DOMException('chart controller destroyed', 'AbortError')); if (this.state.hoverDwellTimer !== null) { clearTimeout(this.state.hoverDwellTimer); diff --git a/web/lib/chart-store.test.ts b/web/lib/chart-store.test.ts index b80516e..3563ea2 100644 --- a/web/lib/chart-store.test.ts +++ b/web/lib/chart-store.test.ts @@ -4,10 +4,12 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { + abortGroupBundle, applyGroupMacro, chartIsHiddenAsEmpty, clearGroupSeriesFilter, ensureGroupBundle, + getCachedPayload, getGlobalFilterSnapshot, getGroupSnapshot, groupSeriesIsVisible, @@ -349,4 +351,46 @@ describe('ensureGroupBundle empty-window classification', () => { expect(chartIsHiddenAsEmpty(snap, 'chart-stale')).toBe(true); expect(chartIsHiddenAsEmpty(snap, 'chart-live')).toBe(false); }); + + it('ignores a late closed bundle without replacing the reopened request or its cached data', async () => { + let resolveOld!: (response: Response) => void; + let resolveNew!: (response: Response) => void; + const fetcher = vi + .fn() + .mockImplementationOnce( + () => + new Promise((resolve) => { + resolveOld = resolve; + }), + ) + .mockImplementationOnce( + () => + new Promise((resolve) => { + resolveNew = resolve; + }), + ); + vi.stubGlobal('fetch', fetcher); + const old = ensureGroupBundle('reopened', 0); + await Promise.resolve(); + abortGroupBundle('reopened'); + const current = ensureGroupBundle('reopened', 0); + await Promise.resolve(); + + const response = (value: number) => + ({ + ok: true, + status: 200, + json: async () => ({ charts: [{ slug: 'cached', series: { vortex: [value] } }] }), + }) as Response; + resolveOld(response(1)); + expect(await old).toEqual({ status: 'aborted' }); + expect(getCachedPayload('cached')).toBeUndefined(); + expect(ensureGroupBundle('reopened', 0)).toBe(current); + resolveNew(response(2)); + expect(await current).toEqual({ status: 'success' }); + expect(getCachedPayload('cached')?.series).toEqual({ vortex: [2] }); + abortGroupBundle('reopened'); + expect(await ensureGroupBundle('reopened', 0)).toEqual({ status: 'success' }); + expect(fetcher).toHaveBeenCalledTimes(2); + }); }); diff --git a/web/lib/chart-store.ts b/web/lib/chart-store.ts index 4229a13..cc85637 100644 --- a/web/lib/chart-store.ts +++ b/web/lib/chart-store.ts @@ -135,27 +135,53 @@ function primePayload(slug: string, payload: ChartResponse): void { payloadCache.set(slug, payload); } -// Group slugs whose bundle fetch already completed successfully (the cache is -// primed for every chart it carried). A reopen of such a group skips the fetch -// entirely, so close/reopen after a success issues zero requests. This survives -// a group close: a successful bundle's cached payloads stay valid for the tab. -const completedBundles = new Set(); - -// Group slugs whose bundle has already been ATTEMPTED in the current open cycle -// (settled as success, 404, or failure). It collapses the eager `armHydration` -// kick and each island's `ensureInitialPayload` re-attempt into a single fetch -// even after the first one settles. Unlike `completedBundles` it is cleared on -// group close (`abortGroupBundle`), so a reopen re-attempts a group whose bundle -// 404'd or failed, while a card still falls back per-chart in the same cycle. -const attemptedBundles = new Set(); +/** Only unavailable bundles and successful bundles missing a chart permit a + * per-chart fallback. Failures require an explicit shared retry. */ +export type BundleResult = + | { status: 'success' } + | { status: 'unavailable' } + | { status: 'failed'; error: unknown } + | { status: 'aborted' }; + +// Successes persist for the session. Other settled outcomes persist until the +// group closes or the user retries, so later cards do not retry passively. +const settledBundles = new Map(); +const bundleRetryListeners = new Map void>>(); + +/** Subscribe a chart to user-initiated retries of its group bundle. */ +export function subscribeGroupBundleRetry(groupSlug: string, cb: () => void): () => void { + let listeners = bundleRetryListeners.get(groupSlug); + if (!listeners) { + listeners = new Set(); + bundleRetryListeners.set(groupSlug, listeners); + } + listeners.add(cb); + return () => { + listeners.delete(cb); + if (listeners.size === 0) { + bundleRetryListeners.delete(groupSlug); + } + }; +} + +/** Retry one failed bundle. Waiting cards join the first subscriber's request. */ +export function retryGroupBundle(groupSlug: string): boolean { + if (settledBundles.get(groupSlug)?.status !== 'failed') { + return false; + } + settledBundles.delete(groupSlug); + for (const cb of bundleRetryListeners.get(groupSlug) ?? []) { + cb(); + } + return true; +} /** Clear the cache and in-flight bundle map. TEST-ONLY: production never evicts * within a tab session. */ export function resetPayloadCache(): void { payloadCache.clear(); inFlightBundles.clear(); - completedBundles.clear(); - attemptedBundles.clear(); + settledBundles.clear(); } /** A group's in-flight bundle fetch: the queue entry (for priority bumps), its @@ -163,7 +189,7 @@ export function resetPayloadCache(): void { interface BundleInFlight { entry: QueueEntry; controller: AbortController; - promise: Promise; + promise: Promise; } const inFlightBundles = new Map(); @@ -172,18 +198,13 @@ const inFlightBundles = new Map(); * Fetch one group's default last-100 bundle (`/api/group/{slug}?n=100`) and * prime [`payloadCache`] for every chart in it. Concurrent callers for the same * group share one in-flight fetch (priority is bumped to the highest caller's). - * A 404 or failure resolves without priming, so callers fall back to the - * per-chart fetch. Never rejects: failures are swallowed here and surfaced as a - * cache miss to the caller. + * A 404 permits per-chart fallback. Other failures remain shared until an + * explicit retry or group reopen. Close cancellation stays silent. */ -export function ensureGroupBundle(groupSlug: string, priority: number): Promise { - // A group whose bundle already succeeded is fully cached; a reopen need not - // refetch (the per-chart cache hit in `ensureInitialPayload` does the rest). - // A group already attempted this open cycle (404 / failure included) is not - // re-fetched either: callers fall back per-chart. Both short-circuits resolve - // immediately so a caller's `.then` still runs and re-checks the cache. - if (completedBundles.has(groupSlug) || attemptedBundles.has(groupSlug)) { - return Promise.resolve(); +export function ensureGroupBundle(groupSlug: string, priority: number): Promise { + const settled = settledBundles.get(groupSlug); + if (settled) { + return Promise.resolve(settled); } const existing = inFlightBundles.get(groupSlug); if (existing) { @@ -196,6 +217,7 @@ export function ensureGroupBundle(groupSlug: string, priority: number): Promise< const url = `/api/group/${encodeURIComponent(groupSlug)}?n=100`; const controller = new AbortController(); const entry = bundleQueue.schedule(async () => { + controller.signal.throwIfAborted(); // The timeout starts when the task actually runs (not while queued), so it // bounds the fetch, not the queue wait. A `TimeoutError` reason lets the // catch tell a timeout apart from a close/destroy `AbortError`. @@ -214,13 +236,21 @@ export function ensureGroupBundle(groupSlug: string, priority: number): Promise< if (!r.ok) { throw new Error(`HTTP ${r.status}`); } - return (await r.json()) as GroupChartsResponse; + const body = (await r.json()) as GroupChartsResponse | null; + if (body === null) { + throw new Error('invalid group bundle'); + } + return body; } finally { clearTimeout(timer); } }, priority); - const promise = entry.promise - .then((body) => { + const promise: Promise = entry.promise + .then((body): BundleResult => { + if (inFlightBundles.get(groupSlug)?.entry !== entry) { + return { status: 'aborted' }; + } + controller.signal.throwIfAborted(); if (body !== null) { const bundle = body as GroupChartsResponse; for (const chart of bundle.charts) { @@ -233,30 +263,29 @@ export function ensureGroupBundle(groupSlug: string, priority: number): Promise< // intersect the viewport (their islands never seed a payload). noteChartRecentData(groupSlug, chart.slug, chartHasRecentData(chart)); } - // Mark the group complete so a reopen short-circuits without a refetch. - // A 404 (`null` body) or a failure leaves it unmarked so a reopen retries. - completedBundles.add(groupSlug); + return { status: 'success' }; } + return { status: 'unavailable' }; }) - .catch((err: unknown) => { - // A close/destroy abort is silent; a timeout or failure leaves the cache - // unprimed so callers fall back per-chart. Surface non-abort failures for - // debugging only. - if (!(err instanceof DOMException && err.name === 'AbortError')) { - console.warn('bench: group bundle fetch failed', err); + .catch((err: unknown): BundleResult => { + if ( + inFlightBundles.get(groupSlug)?.entry !== entry || + (controller.signal.aborted && controller.signal.reason?.name === 'AbortError') + ) { + return { status: 'aborted' }; } + return { + status: 'failed', + error: controller.signal.aborted ? controller.signal.reason : err, + }; }) - .finally(() => { - // Drop the in-flight entry once it settles, but only if a newer fetch has - // not already replaced it (mirrors the per-chart identity-guarded clears). - if (inFlightBundles.get(groupSlug)?.entry === entry) { - inFlightBundles.delete(groupSlug); - // Record that this group was attempted this open cycle so a re-call does - // not re-fetch a just-settled bundle. A close (`abortGroupBundle`) clears - // this so a reopen retries; the in-flight identity guard avoids marking - // a stale (already-replaced) entry's settle. - attemptedBundles.add(groupSlug); + .then((result): BundleResult => { + if (inFlightBundles.get(groupSlug)?.entry !== entry) { + return { status: 'aborted' }; } + inFlightBundles.delete(groupSlug); + settledBundles.set(groupSlug, result); + return result; }); inFlightBundles.set(groupSlug, { entry, controller, promise }); return promise; @@ -264,17 +293,16 @@ export function ensureGroupBundle(groupSlug: string, priority: number): Promise< /** Abort a group's in-flight bundle fetch (on group close) and reset its * per-cycle state so a reopen re-issues. Idempotent. A successfully completed - * bundle's cache stays valid (`completedBundles` is left intact), so a reopen of - * a fully cached group still issues nothing. */ + * bundle's cache stays valid, so a reopen of a fully cached group issues nothing. */ export function abortGroupBundle(groupSlug: string): void { const inFlight = inFlightBundles.get(groupSlug); if (inFlight) { inFlight.controller.abort(new DOMException('group closed', 'AbortError')); inFlightBundles.delete(groupSlug); } - // Clear the per-cycle attempt marker so a reopen re-attempts a group whose - // bundle 404'd or failed; a fully cached group is gated by `completedBundles`. - attemptedBundles.delete(groupSlug); + if (settledBundles.get(groupSlug)?.status !== 'success') { + settledBundles.delete(groupSlug); + } } // ---------------------------------------------------------------------------