diff --git a/frontend/src/__tests__/recommendations.test.ts b/frontend/src/__tests__/recommendations.test.ts index 8a2cd3f2a..8c33e5336 100644 --- a/frontend/src/__tests__/recommendations.test.ts +++ b/frontend/src/__tests__/recommendations.test.ts @@ -1835,16 +1835,133 @@ describe('Recommendations Module', () => { expect(mockShowToast).toHaveBeenCalledWith( expect.objectContaining({ - message: 'Recommendations refresh failed: aws: cost explorer throttled', + message: 'Recommendations refresh failed: the collection reported errors. The previous recommendations are still shown.', kind: 'error', }), ); + // Raw provider error text never reaches the refresh outcome toast. + expect(mockShowToast).not.toHaveBeenCalledWith( + expect.objectContaining({ message: expect.stringMatching(/^Recommendations refresh.*cost explorer throttled/) }), + ); + expect(mockShowToast).not.toHaveBeenCalledWith( + expect.objectContaining({ message: 'Recommendations refreshed' }), + ); + expect(api.getRecommendations).toHaveBeenCalledTimes(2); + }); + + // Issue #1679: in Lambda mode the refresh POST returns 202 as soon as the + // collection is started, so the outcome is only known once the in-flight + // marker clears. A failed new collection must not be reported as refreshed. + test('202 accepted — failure toast, not success, when the started collect failed', async () => { + mockGetRecs(); + (recsApi.getRecommendationsFreshness as jest.Mock).mockResolvedValueOnce({ + last_collected_at: null, + last_collection_error: null, + }); + (recsApi.refreshRecommendations as jest.Mock).mockResolvedValue({ + started_at: new Date().toISOString(), + last_collected_at: null, + }); + (recsApi.getRecommendationsFreshness as jest.Mock).mockResolvedValue({ + last_collected_at: null, + last_collection_error: 'aws: cost explorer throttled', + last_collection_started_at: null, + }); + + await loadRecommendations(); + await Promise.resolve(); + await Promise.resolve(); + await jest.advanceTimersByTimeAsync(5_000); + + expect(mockShowToast).toHaveBeenCalledWith( + expect.objectContaining({ + message: 'Recommendations refresh failed: the collection reported errors. The previous recommendations are still shown.', + kind: 'error', + }), + ); + // Raw provider error text never reaches the refresh outcome toast. + expect(mockShowToast).not.toHaveBeenCalledWith( + expect.objectContaining({ message: expect.stringMatching(/^Recommendations refresh.*cost explorer throttled/) }), + ); + expect(mockShowToast).not.toHaveBeenCalledWith( + expect.objectContaining({ message: 'Recommendations refreshed' }), + ); + expect(api.getRecommendations).toHaveBeenCalledTimes(2); + }); + + test('202 accepted — success toast only after the started collect finishes', async () => { + mockGetRecs(); + (recsApi.getRecommendationsFreshness as jest.Mock).mockResolvedValueOnce({ + last_collected_at: null, + last_collection_error: null, + }); + (recsApi.refreshRecommendations as jest.Mock).mockResolvedValue({ + started_at: new Date().toISOString(), + last_collected_at: null, + }); + let collecting = true; + (recsApi.getRecommendationsFreshness as jest.Mock).mockImplementation(async () => ({ + last_collected_at: collecting ? null : new Date().toISOString(), + last_collection_error: null, + last_collection_started_at: collecting ? new Date().toISOString() : null, + })); + + await loadRecommendations(); + await Promise.resolve(); + await Promise.resolve(); + await jest.advanceTimersByTimeAsync(10_000); + + // Marker still held: nothing announced and no reload yet. expect(mockShowToast).not.toHaveBeenCalledWith( expect.objectContaining({ message: 'Recommendations refreshed' }), ); + expect(api.getRecommendations).toHaveBeenCalledTimes(1); + + collecting = false; + await jest.advanceTimersByTimeAsync(5_000); + + expect(mockShowToast).toHaveBeenCalledWith( + expect.objectContaining({ message: 'Recommendations refreshed', kind: 'success' }), + ); + expect(mockShowToast).not.toHaveBeenCalledWith( + expect.objectContaining({ kind: 'error' }), + ); expect(api.getRecommendations).toHaveBeenCalledTimes(2); }); + test('202 accepted — explicit failure when the started collect outlasts the poll budget', async () => { + mockGetRecs(); + (recsApi.getRecommendationsFreshness as jest.Mock).mockResolvedValueOnce({ + last_collected_at: null, + last_collection_error: null, + }); + (recsApi.refreshRecommendations as jest.Mock).mockResolvedValue({ + started_at: new Date().toISOString(), + last_collected_at: null, + }); + (recsApi.getRecommendationsFreshness as jest.Mock).mockResolvedValue({ + last_collected_at: null, + last_collection_error: null, + last_collection_started_at: new Date().toISOString(), + }); + + await loadRecommendations(); + await Promise.resolve(); + await Promise.resolve(); + await jest.advanceTimersByTimeAsync(60 * 5_000); + + expect(mockShowToast).toHaveBeenCalledWith( + expect.objectContaining({ + message: 'Recommendations refresh is taking longer than expected; try again shortly', + kind: 'error', + }), + ); + expect(mockShowToast).not.toHaveBeenCalledWith( + expect.objectContaining({ message: 'Recommendations refreshed' }), + ); + expect(api.getRecommendations).toHaveBeenCalledTimes(1); + }); + test('dedup — concurrent stale loads fire refreshRecommendations only once', async () => { mockGetRecs(); (recsApi.getRecommendationsFreshness as jest.Mock).mockResolvedValue({ diff --git a/frontend/src/recommendations.ts b/frontend/src/recommendations.ts index 6452527f3..ff4e94040 100644 --- a/frontend/src/recommendations.ts +++ b/frontend/src/recommendations.ts @@ -402,6 +402,8 @@ export async function triggerAutoRefreshIfStale( // POLL_MAX_ATTEMPTS checks total, so the longest wait matches the backend's // own detached-collection budget (5 min in internal/scheduler/scheduler.go). const COLLECTION_POLL_INTERVAL_MS = 5_000; +const REFRESH_FAILED_MESSAGE = + 'Recommendations refresh failed: the collection reported errors. The previous recommendations are still shown.'; const COLLECTION_POLL_MAX_ATTEMPTS = 60; // 60 * 5s = 5 min function delay(ms: number): Promise { @@ -431,8 +433,8 @@ async function pollUntilCollectionClears(): Promise { - inFlight.dismiss(); + // The POST only starts the collection (202 in Lambda mode), and a 409 means + // one is already running; either way the outcome is known only once the + // in-flight marker clears (issue #1679). + const settleWhenCollectionClears = async (): Promise => { + const cleared = await pollUntilCollectionClears(); + inFlight.dismiss(); + if (!cleared) { + showToast({ + message: 'Recommendations refresh is taking longer than expected; try again shortly', + kind: 'error', + }); + return; + } + if (cleared.last_collection_error) { + // last_collection_error can hold raw provider error text, so the toast + // names the outcome only (cf. #748). + showToast({ + message: REFRESH_FAILED_MESSAGE, + kind: 'error', + }); + } else { showToast({ message: 'Recommendations refreshed', kind: 'success', timeout: 5_000, }); - // Reload UI data so users see fresh content. Caller passes a - // page-specific reload (loadDashboard from Home, loadRecommend- - // ations from Opportunities) so we don't force a recs render on - // a page that doesn't display them. - return onReload(); - }) + } + // Reload either way: a partially failed collect may still have persisted + // rows. Callers pass a page-specific reload (loadDashboard from Home, + // loadRecommendations from Opportunities). + await onReload(); + }; + + autoRefreshInFlight = refreshRecommendationsAPI() + .then(settleWhenCollectionClears) .catch(async (err: unknown) => { const status = err !== null && typeof err === 'object' && 'status' in err ? (err as { status?: unknown }).status : undefined; if (status === 409) { - const cleared = await pollUntilCollectionClears(); - if (cleared) { - inFlight.dismiss(); - if (cleared.last_collection_error) { - showToast({ - message: `Recommendations refresh failed: ${cleared.last_collection_error}`, - kind: 'error', - }); - } else { - showToast({ - message: 'Recommendations refreshed', - kind: 'success', - timeout: 5_000, - }); - } - // Reload either way: a partially failed collect may still have - // persisted rows. - await onReload(); - return; - } - inFlight.dismiss(); - showToast({ - message: 'Recommendations refresh is taking longer than expected; try again shortly', - kind: 'error', - }); + await settleWhenCollectionClears(); return; } inFlight.dismiss(); diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index 2956fe24f..1f1b52352 100644 --- a/internal/scheduler/scheduler.go +++ b/internal/scheduler/scheduler.go @@ -203,6 +203,7 @@ func (s *Scheduler) CollectRecommendations(ctx context.Context, ownerToken strin // Get global config globalCfg, err := s.config.GetGlobalConfig(ctx) if err != nil { + s.recordCollectionStoppedBestEffort(collectionStoppedLoadingSettings, err) return nil, err } @@ -238,6 +239,7 @@ func (s *Scheduler) CollectRecommendations(ctx context.Context, ownerToken strin // most 3 providers. allRecommendations, totalSavings, successfulProviders, successfulCollects, failedProviders, partial, err := s.collectAllProviders(ctx, globalCfg) if err != nil { + s.recordCollectionStoppedBestEffort(collectionStoppedIncomplete, err) return nil, err } @@ -461,6 +463,28 @@ func (s *Scheduler) persistCollection(ctx context.Context, recs []config.Recomme s.pruneStaleRecommendationsBestEffort(ctx) } +// Messages recorded when a collection ends before persistCollection runs. They +// are shown to users as the refresh outcome, so they carry no internal detail; +// the cause is logged. +const ( + collectionStoppedLoadingSettings = "recommendations collection stopped before saving: could not load settings" + collectionStoppedIncomplete = "recommendations collection stopped before saving: it was canceled or ran out of time" +) + +// recordCollectionStoppedBestEffort records why a collection ended without +// persisting, before the deferred marker clear runs (#1679). Without it the +// in-flight marker clears with the previous run's (usually empty) error and a +// refresh that collected nothing reads as a success. It uses its own context +// because the run's context may already be done. +func (s *Scheduler) recordCollectionStoppedBestEffort(msg string, cause error) { + logging.Errorf("Recommendations collection stopped before saving: %v", cause) + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + if err := s.config.SetRecommendationsCollectionError(ctx, msg); err != nil { + logging.Errorf("Failed to record collection error: %v", err) + } +} + // upsertWithMarker writes the rows and the collection's partial marker in one // transaction. Both store methods merge the previous Azure pricing hold under // the state row lock, so a complete collection (nil partial) keeps it too. diff --git a/internal/scheduler/scheduler_test.go b/internal/scheduler/scheduler_test.go index 54f0d9758..cbb886828 100644 --- a/internal/scheduler/scheduler_test.go +++ b/internal/scheduler/scheduler_test.go @@ -463,6 +463,11 @@ func TestScheduler_CollectRecommendations_ParallelProviders(t *testing.T) { providerFactory: mockFactory, } + // #1679: a run cut short records why. The write uses its own context, + // since the run's context is already done. + mockStore.On("SetRecommendationsCollectionError", mock.MatchedBy(func(c context.Context) bool { return c.Err() == nil }), + "recommendations collection stopped before saving: it was canceled or ran out of time").Return(nil).Once() + ctx, cancel := context.WithCancel(context.Background()) cancel() // cancel BEFORE the call @@ -686,6 +691,10 @@ func TestScheduler_CollectRecommendations_ConfigError(t *testing.T) { mockEmail := new(MockEmailSender) mockStore.On("GetGlobalConfig", ctx).Return(nil, assert.AnError) + // #1679: the run ends without persisting, so it must say why; otherwise + // the frontend sees the in-flight marker clear with no error and reports + // a refresh that collected nothing as successful. + mockStore.On("SetRecommendationsCollectionError", mock.Anything, "recommendations collection stopped before saving: could not load settings").Return(nil).Once() scheduler := &Scheduler{ config: mockStore,