diff --git a/frontend/src/__tests__/recommendations.test.ts b/frontend/src/__tests__/recommendations.test.ts index 70d2a0c33..a0a1bc07c 100644 --- a/frontend/src/__tests__/recommendations.test.ts +++ b/frontend/src/__tests__/recommendations.test.ts @@ -143,6 +143,12 @@ describe('Recommendations Module', () => { }); (recsApi.refreshRecommendations as jest.Mock).mockResolvedValue({}); mockShowToast.mockReturnValue({ dismiss: jest.fn() }); + + // Reset the auto-refresh in-flight guard AND the once-per-session + // latch so each test starts with a clean auto-refresh state. Without + // this, the first stale-load test latches autoRefreshAttempted and + // every later test in this block would silently skip the auto-refresh. + resetAutoRefreshInFlight(); }); afterEach(() => { @@ -1456,6 +1462,48 @@ describe('Recommendations Module', () => { const inFlightOpts = inFlightCall![0] as { timeout?: number | null }; expect(inFlightOpts.timeout).toBeNull(); }); + + // Runaway-loop regression. Reproduces the production cost incident: the + // backend collect SUCCEEDS (POST /recommendations/refresh resolves) but the + // collector never advances last_collected_at (it fails at a DB error), so + // freshness stays stale forever. The success path calls onReload() + // (loadRecommendations by default), which re-enters triggerAutoRefreshIfStale. + // Pre-fix this recurses and re-fires the paid collect endpoint on every + // reload — observed at ~4-10 calls/sec in production. Post-fix the + // once-per-session latch caps it at exactly one collect across repeated + // loads / the post-refresh reload. + test('runaway loop — stale-that-never-clears fires the collect exactly once across reloads (#cost-risk)', async () => { + mockGetRecs(); + // Freshness is ALWAYS stale (collector never advances it). + (recsApi.getRecommendationsFreshness as jest.Mock).mockResolvedValue({ + last_collected_at: null, + last_collection_error: null, + }); + // The refresh POST SUCCEEDS (real scenario: backend returns 202), which + // is what drives the success-path onReload() re-entry. + (recsApi.refreshRecommendations as jest.Mock).mockResolvedValue({}); + + // First page open: should auto-refresh once. The default onReload is + // loadRecommendations, so the success path re-enters this whole chain. + await loadRecommendations(); + // Drain the .then(onReload) -> loadRecommendations -> triggerAutoRefreshIfStale + // microtask/promise chain several times; pre-fix each drain re-fires the + // collect, so a multi-iteration drain makes the runaway unmistakable. + for (let i = 0; i < 10; i++) { + await Promise.resolve(); + } + + // Even simulated subsequent reloads (filter change, tab re-entry) must + // NOT re-arm the auto-collect within the same session. + await loadRecommendations(); + await loadRecommendations(); + for (let i = 0; i < 10; i++) { + await Promise.resolve(); + } + + // Exactly one paid collect, regardless of how many reloads occurred. + expect(recsApi.refreshRecommendations).toHaveBeenCalledTimes(1); + }); }); describe('openPurchaseModal', () => { diff --git a/frontend/src/recommendations.ts b/frontend/src/recommendations.ts index 680556b80..e7c60611e 100644 --- a/frontend/src/recommendations.ts +++ b/frontend/src/recommendations.ts @@ -85,6 +85,22 @@ let lastRecommendationsSummary: RecommendationsSummary | null = null; // skips kicking off a second request (and duplicate toasts). let autoRefreshInFlight: Promise | null = null; +// Runaway-loop guard: latch so the stale-on-open auto-refresh fires AT MOST +// once per page session. Without it, triggerAutoRefreshIfStale -> (stale) +// -> startRecommendationsRefresh -> onReload() (loadRecommendations / +// loadDashboard) re-enters triggerAutoRefreshIfStale, and when the backend +// collection never advances last_collected_at (e.g. it fails at a DB error, +// so freshness stays stale forever) the success path re-fires the collect +// endlessly — observed at ~4-10 POST /recommendations/refresh per second in +// production, each a paid Cost Explorer / pricing call once the collector +// reaches AWS. autoRefreshInFlight only dedups *concurrent* refreshes; it is +// cleared per cycle, so it cannot stop this sequential re-trigger. The latch +// is intentionally NOT reset on success/failure: the auto-refresh is a +// once-per-load convenience, and a hard page reload resets module state. +// Explicit user actions (lookback change, manual refresh) call +// startRecommendationsRefresh() directly and so are never gated by this latch. +let autoRefreshAttempted = false; + /** * Freshness budget for auto-refresh (#284). If the last successful collection * is older than this threshold (or there has never been one), loadRecommendations @@ -159,6 +175,7 @@ export function resetExpandedCells(): void { */ export function resetAutoRefreshInFlight(): void { autoRefreshInFlight = null; + autoRefreshAttempted = false; } /** @@ -280,6 +297,16 @@ export function setupRecommendationsHandlers(): void { export async function triggerAutoRefreshIfStale( onReload: () => Promise = loadRecommendations, ): Promise { + // Runaway-loop guard: the stale-on-open auto-refresh runs at most once per + // page session. startRecommendationsRefresh's success path calls onReload() + // (loadRecommendations / loadDashboard), which re-enters this function; when + // the collector never advances last_collected_at the freshness stays stale + // and we would re-fire the (paid) collect endlessly. Latch before the + // freshness fetch so neither the GET nor the collect repeats. Explicit user + // refreshes bypass this by calling startRecommendationsRefresh() directly. + if (autoRefreshAttempted) return; + autoRefreshAttempted = true; + let freshness; try { freshness = await getRecommendationsFreshness();