From d27146e7e982c49550402380ecaabb0bb7ce8271 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 8 Jun 2026 18:51:03 -0700 Subject: [PATCH] fix(frontend/recommendations): stop runaway refresh loop hammering the collector triggerAutoRefreshIfStale fired a background re-collect whenever the recommendations cache looked stale, and startRecommendationsRefresh's success path called onReload() (loadRecommendations on Opportunities, loadDashboard on Home), which re-entered triggerAutoRefreshIfStale. 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-fired the collect on every reload with no terminal condition. Observed in production at ~4-10 POST /recommendations/refresh per second, sustained (~18k calls/hour). The calls currently fail fast at a DB error before reaching Cost Explorer, but once the imminent DB fix lands every iteration becomes a paid Cost Explorer / pricing call (~$0.01 each), continuously. The existing autoRefreshInFlight guard only dedups *concurrent* refreshes and is cleared per cycle, so it does not stop this sequential re-trigger. Add a once-per-page-session latch (autoRefreshAttempted) so the stale-on-open auto-refresh fires at most once regardless of how many times loadRecommendations / loadDashboard re-run (filter changes, tab re-entry, the post-refresh reload). The latch is checked before the freshness GET so neither the GET nor the collect repeats. Explicit user actions (lookback change, manual refresh) call startRecommendationsRefresh directly and are never gated by the latch. A hard page reload resets module state, re-arming the once-per-load convenience. Add a regression test that reproduces the real scenario (collect POST resolves, freshness stays stale) and asserts the collect endpoint is hit exactly once across repeated loads and the post-refresh reload. Fails pre-fix (2 calls), passes post-fix (1 call). Reset the latch in the recommendations test beforeEach so the latch cannot leak across tests. --- .../src/__tests__/recommendations.test.ts | 48 +++++++++++++++++++ frontend/src/recommendations.ts | 27 +++++++++++ 2 files changed, 75 insertions(+) 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();