From de566846e01e94564e655af36bcb3e604ad9b193 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 11:53:37 +0100 Subject: [PATCH 1/4] fix(recommendations): report the refresh outcome only after the collection ends POST /recommendations/refresh returns 202 as soon as the collection is started (Lambda mode), but the success path announced "Recommendations refreshed" right away. When the started collection failed, the page showed that success toast next to the collection-error toast. The 2xx path now does what the 409 path already did: poll freshness until the in-flight marker clears, then show success or "refresh failed" from last_collection_error, and reload. Both paths share one helper. Closes #1679 Co-Authored-By: Claude Opus 5.5 --- .../src/__tests__/recommendations.test.ts | 76 +++++++++++++++++++ frontend/src/recommendations.ts | 67 ++++++++-------- 2 files changed, 107 insertions(+), 36 deletions(-) diff --git a/frontend/src/__tests__/recommendations.test.ts b/frontend/src/__tests__/recommendations.test.ts index 8a2cd3f2a..b143820c9 100644 --- a/frontend/src/__tests__/recommendations.test.ts +++ b/frontend/src/__tests__/recommendations.test.ts @@ -1845,6 +1845,82 @@ describe('Recommendations Module', () => { 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: aws: cost explorer throttled', + kind: 'error', + }), + ); + 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('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..433959822 100644 --- a/frontend/src/recommendations.ts +++ b/frontend/src/recommendations.ts @@ -431,8 +431,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) { + showToast({ + message: `Recommendations refresh failed: ${cleared.last_collection_error}`, + 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(); From 7a474b979bbf5949cb251438a6fd97e1ab4615d1 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 14:28:56 +0100 Subject: [PATCH 2/4] fix(scheduler): record why a collection stopped before saving CollectRecommendations returned early when the settings could not be loaded or the provider fan-out ended with a context error, without writing last_collection_error. The deferred marker clear still ran, so the frontend saw the in-flight marker clear with the previous run's (usually empty) error and reported "Recommendations refreshed" for a run that collected nothing (review finding on #1698). Both early returns now record a neutral message before returning; the cause is logged. The write uses its own context, since the run's may be done. Also adds a direct frontend test for the 202 + poll-timeout path. Refs #1679 Co-Authored-By: Claude Opus 5.5 --- .../src/__tests__/recommendations.test.ts | 33 +++++++++++++++++++ internal/scheduler/scheduler.go | 24 ++++++++++++++ internal/scheduler/scheduler_test.go | 9 +++++ 3 files changed, 66 insertions(+) diff --git a/frontend/src/__tests__/recommendations.test.ts b/frontend/src/__tests__/recommendations.test.ts index b143820c9..bd76b74ce 100644 --- a/frontend/src/__tests__/recommendations.test.ts +++ b/frontend/src/__tests__/recommendations.test.ts @@ -1921,6 +1921,39 @@ describe('Recommendations Module', () => { 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/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index 2956fe24f..f16ea1eaa 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 cancelled 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..6f555a9b5 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 cancelled 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, From 713abfd9b5cd5838d768f64ff16ef05592f07df8 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 14:48:13 +0100 Subject: [PATCH 3/4] style(scheduler): spell "canceled" the way misspell expects Refs #1679 Co-Authored-By: Claude Opus 5.5 --- internal/scheduler/scheduler.go | 2 +- internal/scheduler/scheduler_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index f16ea1eaa..1f1b52352 100644 --- a/internal/scheduler/scheduler.go +++ b/internal/scheduler/scheduler.go @@ -468,7 +468,7 @@ func (s *Scheduler) persistCollection(ctx context.Context, recs []config.Recomme // the cause is logged. const ( collectionStoppedLoadingSettings = "recommendations collection stopped before saving: could not load settings" - collectionStoppedIncomplete = "recommendations collection stopped before saving: it was cancelled or ran out of time" + collectionStoppedIncomplete = "recommendations collection stopped before saving: it was canceled or ran out of time" ) // recordCollectionStoppedBestEffort records why a collection ended without diff --git a/internal/scheduler/scheduler_test.go b/internal/scheduler/scheduler_test.go index 6f555a9b5..cbb886828 100644 --- a/internal/scheduler/scheduler_test.go +++ b/internal/scheduler/scheduler_test.go @@ -466,7 +466,7 @@ func TestScheduler_CollectRecommendations_ParallelProviders(t *testing.T) { // #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 cancelled or ran out of time").Return(nil).Once() + "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 From 048042f8d469808c6ea5c3034ec31dd6f6e663f1 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 15:33:23 +0100 Subject: [PATCH 4/4] fix(recommendations): keep provider error text out of the refresh toast last_collection_error can hold raw provider error text, and the refresh outcome toast (202 and 409 paths) printed it verbatim. The toast now says the collection reported errors and that the previous recommendations are still shown. The server-side summary is tracked separately. Refs #1679 Co-Authored-By: Claude Opus 5.5 --- frontend/src/__tests__/recommendations.test.ts | 12 ++++++++++-- frontend/src/recommendations.ts | 6 +++++- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/frontend/src/__tests__/recommendations.test.ts b/frontend/src/__tests__/recommendations.test.ts index bd76b74ce..8c33e5336 100644 --- a/frontend/src/__tests__/recommendations.test.ts +++ b/frontend/src/__tests__/recommendations.test.ts @@ -1835,10 +1835,14 @@ 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' }), ); @@ -1871,10 +1875,14 @@ 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' }), ); diff --git a/frontend/src/recommendations.ts b/frontend/src/recommendations.ts index 433959822..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 { @@ -473,8 +475,10 @@ export function startRecommendationsRefresh( 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: `Recommendations refresh failed: ${cleared.last_collection_error}`, + message: REFRESH_FAILED_MESSAGE, kind: 'error', }); } else {