fix(recommendations): report the refresh outcome only after the collection ends

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({

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<void> {
@@ -431,8 +433,8 @@ async function pollUntilCollectionClears(): Promise<RecommendationsFreshness | n
 
 /**
  * Kick off a recommendations re-collect, surface the three-stage toast
- * (in-flight / success / failure), and reload the page-specific UI on
- * success. Dedups against a refresh already in flight (#284) so the
+ * (in-flight / success / failure), and reload the page-specific UI once the
+ * collection finishes. Dedups against a refresh already in flight (#284) so the
  * stale-on-open path and the lookback-change path (#909) can never run
  * two concurrent collects. Returns the in-flight promise (or the existing
  * one when a refresh is already running) so callers that need to await
@@ -459,50 +461,47 @@ export function startRecommendationsRefresh(
     timeout: null,
   });
 
-  autoRefreshInFlight = refreshRecommendationsAPI()
-    .then(() => {
-      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<void> => {
+    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();

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.

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,