From 8e73bac7075b627d459101a1b5d1b6f4a07b5bed Mon Sep 17 00:00:00 2001 From: Peli de Halleux Date: Wed, 29 Jul 2026 08:47:44 +0000 Subject: [PATCH 1/2] perf(forecast): eliminate full lock-file parsing and add negative caching; fix spinner flicker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Speeds up `gh aw forecast` from ~7.3s to ~2.85s (user CPU 4.0s → 0.18s) and removes concurrent-spinner flicker. Performance - workflow.GetAllWorkflows fully YAML-parsed every .lock.yml (~265 files, ~36MB) just to read the top-level `name:`, costing ~2.5s (called per forecast workflow via GetWorkflowLockFileName). Replace with extractLockFileWorkflowName, which scans for the column-0 `name:` line and parses only that line, with early termination. - Runs with no usage artifact / no AIC data were re-fetched over the network on every forecast. Add negative caching (no_data marker) so completed runs with no AIC data are skipped on subsequent runs. Transient failures (cancellation, download errors) are not negatively cached. UX - console.SpinnerWrapper now coordinates globally: only one spinner renders at a time. Concurrent Start() calls are suppressed (lifecycle tracked but not animated), eliminating flicker from nested/parallel spinners. The terminal slot is released on Stop() and on self-exit. Tests - Add spinner concurrency-suppression test, negative-cache round-trip/version tests, and realistic/unquoted lock-file name-extraction tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 456d730a-a0a6-449f-8adf-5aa845107def --- pkg/cli/forecast_cache.go | 42 ++++++++++++--- pkg/cli/forecast_cache_test.go | 33 ++++++++++++ pkg/cli/forecast_compute.go | 8 +++ pkg/console/spinner.go | 98 +++++++++++++++++++++++++++++----- pkg/console/spinner_test.go | 48 +++++++++++++++++ pkg/workflow/resolve.go | 60 ++++++++++++++++----- pkg/workflow/resolve_test.go | 40 ++++++++++++++ 7 files changed, 295 insertions(+), 34 deletions(-) diff --git a/pkg/cli/forecast_cache.go b/pkg/cli/forecast_cache.go index 50fb37778b5..0654c6553b7 100644 --- a/pkg/cli/forecast_cache.go +++ b/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) } diff --git a/pkg/cli/forecast_cache_test.go b/pkg/cli/forecast_cache_test.go index 7558c01dc17..b231467eb06 100644 --- a/pkg/cli/forecast_cache_test.go +++ b/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.Equal(t, 0.0, got) + + // 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") + } +} diff --git a/pkg/cli/forecast_compute.go b/pkg/cli/forecast_compute.go index 01137cad412..834b6955ac8 100644 --- a/pkg/cli/forecast_compute.go +++ b/pkg/cli/forecast_compute.go @@ -291,18 +291,23 @@ func loadCachedRunAIC(ctx context.Context, runID int64, verbose bool) float64 { if err := tryDownload(usageFilter); err != nil { if errors.Is(err, ErrNoArtifacts) { 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 } } @@ -310,6 +315,9 @@ func loadCachedRunAIC(ctx context.Context, runID int64, verbose bool) float64 { 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) diff --git a/pkg/console/spinner.go b/pkg/console/spinner.go index f92d2505064..cc80c5d7dbc 100644 --- a/pkg/console/spinner.go +++ b/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,12 +171,21 @@ 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") @@ -145,8 +193,11 @@ func (s *SpinnerWrapper) Start() { defer s.wg.Done() defer func() { s.mu.Lock() - defer s.mu.Unlock() s.running = false + s.mu.Unlock() + // Release the terminal when the program exits on its own (e.g. panic or + // self-quit) so a subsequent spinner can claim and render. + releaseActiveSpinner(s) }() defer func() { if r := recover(); r != nil { @@ -160,6 +211,7 @@ 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() @@ -167,19 +219,28 @@ func (s *SpinnerWrapper) Stop() { 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() @@ -187,12 +248,20 @@ func (s *SpinnerWrapper) StopWithMessage(msg string) { 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. + releaseActiveSpinner(s) + fmt.Fprintf(s.out, "%s\n", msg) + } } else { // Still print the message even if spinner wasn't running fmt.Fprintf(s.out, "%s\n", msg) @@ -208,7 +277,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)) diff --git a/pkg/console/spinner_test.go b/pkg/console/spinner_test.go index 9a7ffd8809a..df46af005a8 100644 --- a/pkg/console/spinner_test.go +++ b/pkg/console/spinner_test.go @@ -286,3 +286,51 @@ func TestSpinnerStopBeforeStartRaceCondition(t *testing.T) { spinner.StopWithMessage("Done") } } + +// TestSpinnerGlobalCoordinationSuppressesConcurrent verifies that only one spinner +// renders at a time: when a second spinner Start()s while another is active, it is +// suppressed (does not claim the terminal) to avoid concurrent-spinner flicker. +func TestSpinnerGlobalCoordinationSuppressesConcurrent(t *testing.T) { + first := &SpinnerWrapper{enabled: true, out: io.Discard, program: newNoopSpinnerProgram()} + second := &SpinnerWrapper{enabled: true, out: io.Discard, program: newNoopSpinnerProgram()} + + first.Start() + defer first.Stop() + second.Start() + defer second.Stop() + + first.mu.Lock() + firstSuppressed := first.suppressed + first.mu.Unlock() + second.mu.Lock() + secondSuppressed := second.suppressed + second.mu.Unlock() + + if firstSuppressed { + t.Error("first spinner should own the terminal, not be suppressed") + } + if !secondSuppressed { + t.Error("second concurrent spinner should be suppressed while the first is active") + } + + // After the first stops, a newly started spinner should be able to claim the terminal. + first.Stop() + third := &SpinnerWrapper{enabled: true, out: io.Discard, program: newNoopSpinnerProgram()} + third.Start() + defer third.Stop() + third.mu.Lock() + thirdSuppressed := third.suppressed + third.mu.Unlock() + if thirdSuppressed { + t.Error("third spinner should claim the terminal once the first has released it") + } +} + +func newNoopSpinnerProgram() *tea.Program { + return tea.NewProgram( + spinnerModel{message: "test", output: io.Discard}, + tea.WithOutput(io.Discard), + tea.WithoutRenderer(), + tea.WithInput(nil), + ) +} diff --git a/pkg/workflow/resolve.go b/pkg/workflow/resolve.go index a8adf5e90a9..86a0989f1e5 100644 --- a/pkg/workflow/resolve.go +++ b/pkg/workflow/resolve.go @@ -1,6 +1,7 @@ package workflow import ( + "bufio" "errors" "fmt" "os" @@ -245,36 +246,67 @@ func GetAllWorkflows() ([]WorkflowNameMatch, error) { base := filepath.Base(lockFile) workflowID := strings.TrimSuffix(base, ".lock.yml") - // Read and parse the lock file to get display name - content, err := os.ReadFile(lockFile) + // Extract only the top-level `name:` field. Lock files are large generated + // GitHub Actions YAML (hundreds of KB each); fully parsing every file just to + // read one field is very slow (seconds across a repo with many workflows), so + // we scan for the first column-0 `name:` line and parse only that. + name, err := extractLockFileWorkflowName(lockFile) if err != nil { - resolveLog.Printf("Failed to read lock file %s: %v", lockFile, err) + resolveLog.Printf("Failed to read workflow name from lock file %s: %v", lockFile, err) continue } - var wf struct { - Name string `yaml:"name"` - } - - if err := yaml.Unmarshal(content, &wf); err != nil { - resolveLog.Printf("Failed to parse YAML from lock file %s: %v", lockFile, err) - continue - } - - if wf.Name == "" { + if name == "" { resolveLog.Printf("Workflow name field missing in lock file: %s", lockFile) continue } workflows = append(workflows, WorkflowNameMatch{ WorkflowID: workflowID, - DisplayName: wf.Name, + DisplayName: name, }) } return workflows, nil } +// extractLockFileWorkflowName returns the top-level workflow `name:` value from a +// generated lock file without parsing the entire (potentially very large) YAML +// document. It scans line-by-line for the first column-0 `name:` key — which in +// generated lock files is always the workflow name — and unmarshals just that single +// line so YAML scalar semantics (quoting, escapes) are preserved. Scanning stops as +// soon as the name is found. +func extractLockFileWorkflowName(lockFile string) (string, error) { + f, err := os.Open(lockFile) + if err != nil { + return "", err + } + defer f.Close() + + scanner := bufio.NewScanner(f) + // Lock file `name:` lines are short, but allow generous headroom for long names. + scanner.Buffer(make([]byte, 0, 64*1024), 1024*1024) + for scanner.Scan() { + line := scanner.Text() + // Only a column-0 (unindented, non-comment) `name:` key is the workflow name; + // nested `name:` keys under jobs/steps are always indented. + if !strings.HasPrefix(line, "name:") { + continue + } + var wf struct { + Name string `yaml:"name"` + } + if err := yaml.Unmarshal([]byte(line), &wf); err != nil { + return "", err + } + return wf.Name, nil + } + if err := scanner.Err(); err != nil { + return "", err + } + return "", nil +} + // IsIntentionalFailure reports whether the workflow identified by workflowPath is tagged // with intentional-failure: true in its frontmatter. workflowPath may be: // - a .lock.yml path (e.g. ".github/workflows/daily-credit-limit-test.lock.yml") diff --git a/pkg/workflow/resolve_test.go b/pkg/workflow/resolve_test.go index b6fb45bd0d2..54ef4fe706a 100644 --- a/pkg/workflow/resolve_test.go +++ b/pkg/workflow/resolve_test.go @@ -578,3 +578,43 @@ func TestIsIntentionalFailure(t *testing.T) { }) } } + +// TestGetAllWorkflows_RealisticLockFileShape verifies that the workflow name is +// extracted from the top-level `name:` key even when the lock file has a large +// comment header and many indented nested `name:` keys (jobs/steps). This guards the +// performance optimization that avoids fully parsing each lock file. +func TestGetAllWorkflows_RealisticLockFileShape(t *testing.T) { + workflowsDir := setupWorkflowDir(t) + + lockContent := `# gh-aw-metadata: {"schema_version":"v4"} +# This file was automatically generated by gh-aw. DO NOT EDIT. +# +name: "Smoke Copilot" +on: + push: +jobs: + build: + name: nested-job-name + steps: + - name: nested step name + run: echo hi +` + require.NoError(t, os.WriteFile(filepath.Join(workflowsDir, "smoke-copilot.lock.yml"), []byte(lockContent), 0644)) + + workflows, err := GetAllWorkflows() + require.NoError(t, err) + require.Len(t, workflows, 1) + assert.Equal(t, "smoke-copilot", workflows[0].WorkflowID) + assert.Equal(t, "Smoke Copilot", workflows[0].DisplayName, "must extract the top-level name, not a nested job/step name") +} + +// TestGetAllWorkflows_UnquotedName verifies plain (unquoted) scalar names are handled. +func TestGetAllWorkflows_UnquotedName(t *testing.T) { + workflowsDir := setupWorkflowDir(t) + require.NoError(t, os.WriteFile(filepath.Join(workflowsDir, "plain.lock.yml"), []byte("name: Plain Name\non: push\n"), 0644)) + + workflows, err := GetAllWorkflows() + require.NoError(t, err) + require.Len(t, workflows, 1) + assert.Equal(t, "Plain Name", workflows[0].DisplayName) +} From 9fc2b99f2277281ff7ca5e6127d010c73901c6b2 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 29 Jul 2026 09:13:22 +0000 Subject: [PATCH 2/2] fix: lint float-compare, spinner race, StopWithMessage flicker, ErrNoArtifacts over-caching Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/forecast_cache_test.go | 2 +- pkg/cli/forecast_compute.go | 20 +++++++++++++++----- pkg/cli/forecast_test.go | 2 +- pkg/console/spinner.go | 11 +++++++---- 4 files changed, 24 insertions(+), 11 deletions(-) diff --git a/pkg/cli/forecast_cache_test.go b/pkg/cli/forecast_cache_test.go index b231467eb06..3a024dfab77 100644 --- a/pkg/cli/forecast_cache_test.go +++ b/pkg/cli/forecast_cache_test.go @@ -74,7 +74,7 @@ func TestForecastAICCache_NegativeCacheRoundTrip(t *testing.T) { saveForecastNoDataCache(dir, runID) got, ok := loadForecastAICCache(dir, runID) require.True(t, ok, "expected negative-cache hit after saving no-data marker") - assert.Equal(t, 0.0, got) + 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)) diff --git a/pkg/cli/forecast_compute.go b/pkg/cli/forecast_compute.go index 834b6955ac8..1ef5ddaa74f 100644 --- a/pkg/cli/forecast_compute.go +++ b/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,7 +296,7 @@ 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. @@ -333,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. @@ -381,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 { diff --git a/pkg/cli/forecast_test.go b/pkg/cli/forecast_test.go index e0f3590ff86..baf29c02d1b 100644 --- a/pkg/cli/forecast_test.go +++ b/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 diff --git a/pkg/console/spinner.go b/pkg/console/spinner.go index cc80c5d7dbc..ef6df1624d6 100644 --- a/pkg/console/spinner.go +++ b/pkg/console/spinner.go @@ -193,11 +193,12 @@ func (s *SpinnerWrapper) Start() { defer s.wg.Done() defer func() { s.mu.Lock() + // 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() - // Release the terminal when the program exits on its own (e.g. panic or - // self-quit) so a subsequent spinner can claim and render. - releaseActiveSpinner(s) }() defer func() { if r := recover(); r != nil { @@ -259,8 +260,10 @@ func (s *SpinnerWrapper) StopWithMessage(msg string) { 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\n", msg) + fmt.Fprintf(s.out, "%s%s%s\n", ansiCarriageReturn, ansiClearLine, msg) } } else { // Still print the message even if spinner wasn't running