perf(forecast): drop full lock-file parsing, add negative caching, fix spinner flicker

pkg/cli/forecast_cache.go

@@ -24,13 +24,16 @@ type forecastAICCache struct {
 	CLIVersion string    `json:"cli_version"` // CLI version used to compute the AIC (cache invalidation key)
 	RunID      int64     `json:"run_id"`      // Workflow run database ID
 	AIC        float64   `json:"ai_credits"`  // Total AI Credits consumed by the run
+	NoData     bool      `json:"no_data"`     // True when the run has no usage artifact / no AIC data (negative cache)
 	CachedAt   time.Time `json:"cached_at"`   // When this cache entry was written
 }
 
 // loadForecastAICCache returns the cached AIC for a run when a valid, version-matching
 // forecast cache file exists. The second return value reports whether a usable cache
-// entry was found. Stale entries (version mismatch, mismatched run ID, or non-positive
-// AIC) are treated as misses so the caller recomputes.
+// entry was found. A negative-cache entry (NoData) is a hit that returns 0, letting the
+// caller skip the network for runs that are known to have no AIC data. Stale entries
+// (version mismatch, mismatched run ID, or a positive-AIC entry that is somehow <= 0)
+// are treated as misses so the caller recomputes.
 func loadForecastAICCache(dir string, runID int64) (float64, bool) {
 	path := filepath.Join(dir, forecastAICCacheFileName)
 	data, err := os.ReadFile(path)
@@ -41,7 +44,15 @@ func loadForecastAICCache(dir string, runID int64) (float64, bool) {
 	if err := json.Unmarshal(data, &c); err != nil {
 		return 0, false
 	}
-	if c.CLIVersion != GetVersion() || c.RunID != runID || c.AIC <= 0 {
+	if c.CLIVersion != GetVersion() || c.RunID != runID {
+		return 0, false
+	}
+	// Negative cache: the run is known to have no AIC data. Report a hit so the caller
+	// short-circuits and avoids re-downloading the (absent) usage artifact every run.
+	if c.NoData {
+		return 0, true
+	}
+	if c.AIC <= 0 {
 		return 0, false
 	}
 	return c.AIC, true
@@ -55,12 +66,31 @@ func saveForecastAICCache(dir string, runID int64, aic float64) {
 	if aic <= 0 {
 		return
 	}
-	c := forecastAICCache{
+	writeForecastAICCache(dir, runID, forecastAICCache{
 		CLIVersion: GetVersion(),
 		RunID:      runID,
 		AIC:        aic,
 		CachedAt:   time.Now().UTC(),
-	}
+	})
+}
+
+// saveForecastNoDataCache writes a negative-cache marker for a run that has no usage
+// artifact or no AIC data. This lets subsequent forecast runs skip the (repeated,
+// definitively empty) network lookups for completed runs that will never yield AIC
+// data. Only call this for permanent no-data conditions — never for transient errors
+// such as context cancellation or download failures.
+func saveForecastNoDataCache(dir string, runID int64) {
+	writeForecastAICCache(dir, runID, forecastAICCache{
+		CLIVersion: GetVersion(),
+		RunID:      runID,
+		NoData:     true,
+		CachedAt:   time.Now().UTC(),
+	})
+}
+
+// writeForecastAICCache serialises c to the run's forecast cache file. Best-effort:
+// all errors are logged and swallowed so a cache-write failure never fails the forecast.
+func writeForecastAICCache(dir string, runID int64, c forecastAICCache) {
 	data, err := json.MarshalIndent(&c, "", "  ")
 	if err != nil {
 		forecastRunLog.Printf("Failed to marshal forecast AIC cache for run %d: %v", runID, err)
@@ -75,5 +105,5 @@ func saveForecastAICCache(dir string, runID int64, aic float64) {
 		forecastRunLog.Printf("Failed to write forecast AIC cache for run %d: %v", runID, err)
 		return
 	}
-	forecastRunLog.Printf("Wrote forecast AIC cache for run %d: aic=%.3f (path=%s)", runID, aic, path)
+	forecastRunLog.Printf("Wrote forecast AIC cache for run %d: aic=%.3f, no_data=%t (path=%s)", runID, c.AIC, c.NoData, path)
 }

pkg/cli/forecast_cache_test.go

@@ -65,3 +65,36 @@ func TestForecastAICCache_MismatchedRunID(t *testing.T) {
 		t.Fatalf("expected cache miss when run ID differs")
 	}
 }
+
+func TestForecastAICCache_NegativeCacheRoundTrip(t *testing.T) {
+	dir := t.TempDir()
+	const runID int64 = 555
+
+	// A no-data marker is a cache hit that reports AIC 0, letting the caller skip the network.
+	saveForecastNoDataCache(dir, runID)
+	got, ok := loadForecastAICCache(dir, runID)
+	require.True(t, ok, "expected negative-cache hit after saving no-data marker")
+	assert.InDelta(t, 0.0, got, 1e-9)
+
+	// The marker records NoData=true and the current CLI version.
+	data, err := os.ReadFile(filepath.Join(dir, forecastAICCacheFileName))
+	require.NoError(t, err)
+	var c forecastAICCache
+	require.NoError(t, json.Unmarshal(data, &c))
+	assert.True(t, c.NoData)
+	assert.Equal(t, GetVersion(), c.CLIVersion)
+	assert.Equal(t, runID, c.RunID)
+}
+
+func TestForecastAICCache_NegativeCacheInvalidatedOnVersionMismatch(t *testing.T) {
+	dir := t.TempDir()
+	const runID int64 = 777
+	c := forecastAICCache{CLIVersion: "old", RunID: runID, NoData: true}
+	data, err := json.MarshalIndent(&c, "", "  ")
+	require.NoError(t, err)
+	require.NoError(t, os.WriteFile(filepath.Join(dir, forecastAICCacheFileName), data, 0o644))
+
+	if _, ok := loadForecastAICCache(dir, runID); ok {
+		t.Fatalf("expected miss for stale-version no-data marker so the run is re-checked")
+	}
+}

pkg/cli/forecast_compute.go

@@ -27,6 +27,13 @@ import (
 // and rate-limit headroom.
 const defaultForecastDownloadConcurrency = 8
 
+// errNoMatchingArtifact is returned by forecastDownloadUsageArtifact when
+// the artifact listing succeeds but no artifact name matches the requested
+// filter.  It is distinct from ErrNoArtifacts, which is used when a download
+// is attempted but the output directory ends up empty (a transient failure
+// that should not be negatively cached).
+var errNoMatchingArtifact = errors.New("no matching artifact found for filter")
+
 var (
 	forecastLoadCachedRunAIC = loadCachedRunAIC
 	// forecastDownloadRunArtifacts uses a forecast-specific implementation that downloads
@@ -289,27 +296,35 @@ func loadCachedRunAIC(ctx context.Context, runID int64, verbose bool) float64 {
 	}
 	usageFilter := []string{"usage"}
 	if err := tryDownload(usageFilter); err != nil {
-		if errors.Is(err, ErrNoArtifacts) {
+		if errors.Is(err, errNoMatchingArtifact) {
 			forecastRunLog.Printf("No usage artifact for run %d; AIC will be 0", runID)
+			// Negative-cache this completed run so future forecasts don't re-list its
+			// (nonexistent) artifacts over the network on every invocation.
+			saveForecastNoDataCache(dir, runID)
 			return 0
 		} else if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) {
 			forecastRunLog.Printf("Usage artifact download for run %d interrupted: %v", runID, err)
 			if verbose {
 				fmt.Fprintln(os.Stderr, console.FormatVerboseMessage(fmt.Sprintf("Usage artifact download for run %d interrupted: %v", runID, err)))
 			}
+			// Transient interruption — do NOT negative-cache; retry next run.
 			return 0
 		} else {
 			forecastRunLog.Printf("Failed to download usage artifact for run %d: %v", runID, err)
 			if verbose {
 				fmt.Fprintln(os.Stderr, console.FormatVerboseMessage(fmt.Sprintf("Failed to download usage artifact for run %d: %v", runID, err)))
 			}
+			// Transient/download failure — do NOT negative-cache; retry next run.
 			return 0
 		}
 	}
 
 	tokenUsage, err := forecastAnalyzeTokenUsage(dir, verbose)
 	if err != nil || tokenUsage == nil || tokenUsage.TotalAIC <= 0 {
 		forecastRunLog.Printf("No AIC data in usage artifact for run %d (err=%v, tokenUsage=%v)", runID, err, tokenUsage)
+		// The usage artifact was fetched but carries no AIC data; this is permanent for a
+		// completed run, so negative-cache it to skip the download next time.
+		saveForecastNoDataCache(dir, runID)
 		return 0
 	}
 	forecastRunLog.Printf("AIC from usage artifact for run %d: aic=%.3f", runID, tokenUsage.TotalAIC)
@@ -325,8 +340,10 @@ func loadCachedRunAIC(ctx context.Context, runID int64, verbose bool) float64 {
 //   - Skips workflow run log downloads entirely — logs are not needed for
 //     AIC computation and downloading them wastes time when forecasting
 //     many runs.
-//   - Returns ErrNoArtifacts immediately when no matching artifact is found
-//     rather than falling back to log diagnostics.
+//   - Returns errNoMatchingArtifact when the listing succeeds but no artifact
+//     name matches the filter (safe to negatively cache — the run has no usage
+//     data). Returns ErrNoArtifacts when a listed artifact was attempted but the
+//     output directory is empty after download (transient; must not be cached).
 //
 // It is referenced by forecastDownloadRunArtifacts so that tests can substitute
 // a mock implementation without modifying the general artifact download path.
@@ -373,11 +390,12 @@ func forecastDownloadUsageArtifact(ctx context.Context, runID int64, outputDir s
 	forecastRunLog.Printf("Run %d: listed artifacts=%v, filter=%v, downloadable=%v", runID, artifactNames, artifactFilter, downloadableNames)
 
 	if len(downloadableNames) == 0 {
-		// No usage artifact — clean up empty directory and report.
+		// Listing succeeded but no artifact matches the filter; clean up the empty
+		// directory and return the distinct sentinel so the caller can negatively cache.
 		if fileutil.IsDirEmpty(outputDir) {
 			_ = os.RemoveAll(outputDir)
 		}
-		return ErrNoArtifacts
+		return errNoMatchingArtifact
 	}
 
 	if shouldLogProgress {

pkg/cli/forecast_test.go

@@ -468,7 +468,7 @@ func TestLoadCachedRunAIC_MissingUsageReturnsZero(t *testing.T) {
 	analyzeCalled := false
 	forecastDownloadRunArtifacts = func(_ context.Context, _ int64, _ string, _ bool, _, _, _ string, artifactFilter []string) error {
 		downloaded = append(downloaded, strings.Join(artifactFilter, ","))
-		return ErrNoArtifacts
+		return errNoMatchingArtifact
 	}
 	forecastAnalyzeTokenUsage = func(_ string, _ bool) (*TokenUsageSummary, error) {
 		analyzeCalled = true

pkg/console/spinner.go

@@ -90,12 +90,51 @@ func (m spinnerModel) render() {
 
 // SpinnerWrapper wraps the spinner functionality with TTY detection and Bubble Tea program
 type SpinnerWrapper struct {
-	program *tea.Program
-	out     io.Writer
-	enabled bool
-	running bool
-	mu      sync.Mutex
-	wg      sync.WaitGroup
+	program    *tea.Program
+	out        io.Writer
+	enabled    bool
+	running    bool
+	suppressed bool
+	mu         sync.Mutex
+	wg         sync.WaitGroup
+}
+
+// Global spinner coordination.
+//
+// Only a single spinner may render to the terminal at a time. Multiple concurrent
+// spinners (e.g. an outer "Sampling…" spinner plus several parallel per-run
+// "Downloading…" spinners) would otherwise each write carriage-return/clear-line
+// escape sequences to the same stderr line, producing visible flicker.
+//
+// The first spinner to Start() claims the terminal and animates; any spinner that
+// Start()s while another is already active is "suppressed" — it tracks its running
+// lifecycle so Stop()/UpdateMessage() stay well-behaved, but it does not render.
+var (
+	globalSpinnerMu     sync.Mutex
+	globalSpinnerActive *SpinnerWrapper
+)
+
+// claimActiveSpinner attempts to make s the single actively-rendering spinner.
+// It returns true when s becomes active, or false when another spinner already owns
+// the terminal (in which case s should suppress its rendering).
+func claimActiveSpinner(s *SpinnerWrapper) bool {
+	globalSpinnerMu.Lock()
+	defer globalSpinnerMu.Unlock()
+	if globalSpinnerActive != nil {
+		return false
+	}
+	globalSpinnerActive = s
+	return true
+}
+
+// releaseActiveSpinner clears s as the active spinner. It is a no-op when s never
+// claimed the terminal (i.e. it was suppressed), so it is always safe to call.
+func releaseActiveSpinner(s *SpinnerWrapper) {
+	globalSpinnerMu.Lock()
+	defer globalSpinnerMu.Unlock()
+	if globalSpinnerActive == s {
+		globalSpinnerActive = nil
+	}
 }
 
 // NewSpinner creates a new spinner with the given message using MiniDot style.
@@ -132,21 +171,34 @@ func (s *SpinnerWrapper) Start() {
 			if s.running {
 				return false
 			}
+			// Only one spinner renders at a time. If another spinner already owns the
+			// terminal, mark this one suppressed: it stays "running" for lifecycle
+			// bookkeeping but does not animate, avoiding concurrent-spinner flicker.
+			if !claimActiveSpinner(s) {
+				s.suppressed = true
+				s.running = true
+				return false
+			}
+			s.suppressed = false
 			s.running = true
 			s.wg.Add(1)
 			return true
 		}()
 		if !shouldStart {
-			spinnerLog.Print("Spinner already running, skipping Start")
+			spinnerLog.Print("Spinner already running or suppressed by an active spinner, skipping render")
 			return
 		}
 		spinnerLog.Print("Starting spinner")
 		go func() {
 			defer s.wg.Done()
 			defer func() {
 				s.mu.Lock()
-				defer s.mu.Unlock()
+				// Release the terminal slot before exposing the stopped state so a
+				// concurrent Start() cannot observe running=false while this wrapper
+				// still owns the global slot and become permanently suppressed.
+				releaseActiveSpinner(s)
 				s.running = false
+				s.mu.Unlock()
 			}()
 			defer func() {
 				if r := recover(); r != nil {
@@ -160,39 +212,59 @@ func (s *SpinnerWrapper) Start() {
 
 func (s *SpinnerWrapper) Stop() {
 	if s.enabled && s.program != nil {
+		var wasRendering bool
 		wasRunning := func() bool {
 			s.mu.Lock()
 			defer s.mu.Unlock()
 			if !s.running {
 				return false
 			}
 			s.running = false
+			wasRendering = !s.suppressed
+			s.suppressed = false
 			return true
 		}()
-		if wasRunning {
+		if !wasRunning {
+			return
+		}
+		if wasRendering {
 			spinnerLog.Print("Stopping spinner")
 			s.program.Quit()
-			s.wg.Wait() // Wait for the goroutine to complete
+			s.wg.Wait() // Wait for the goroutine to complete (goroutine releases the slot)
 			fmt.Fprintf(s.out, "%s%s", ansiCarriageReturn, ansiClearLine)
+		} else {
+			// Suppressed spinner has no goroutine; release the slot directly (no-op).
+			releaseActiveSpinner(s)
 		}
 	}
 }
 
 func (s *SpinnerWrapper) StopWithMessage(msg string) {
 	if s.enabled && s.program != nil {
+		var wasRendering bool
 		wasRunning := func() bool {
 			s.mu.Lock()
 			defer s.mu.Unlock()
 			if !s.running {
 				return false
 			}
 			s.running = false
+			wasRendering = !s.suppressed
+			s.suppressed = false
 			return true
 		}()
 		if wasRunning {
-			s.program.Quit()
-			s.wg.Wait() // Wait for the goroutine to complete
-			fmt.Fprintf(s.out, "%s%s%s\n", ansiCarriageReturn, ansiClearLine, msg)
+			if wasRendering {
+				s.program.Quit()
+				s.wg.Wait() // Wait for the goroutine to complete (goroutine releases the slot)
+				fmt.Fprintf(s.out, "%s%s%s\n", ansiCarriageReturn, ansiClearLine, msg)
+			} else {
+				// Suppressed spinner never rendered; release the slot and print the message.
+				// Still emit CR+ClearLine so the message is not appended to any active
+				// spinner frame that may currently occupy the terminal line.
+				releaseActiveSpinner(s)
+				fmt.Fprintf(s.out, "%s%s%s\n", ansiCarriageReturn, ansiClearLine, msg)
+			}
 		} else {
 			// Still print the message even if spinner wasn't running
 			fmt.Fprintf(s.out, "%s\n", msg)
@@ -208,7 +280,8 @@ func (s *SpinnerWrapper) UpdateMessage(message string) {
 		running := func() bool {
 			s.mu.Lock()
 			defer s.mu.Unlock()
-			return s.running
+			// Suppressed spinners have no live program to receive the update.
+			return s.running && !s.suppressed
 		}()
 		if running {
 			s.program.Send(updateMessageMsg(message))