perf(forecast): drop full lock-file parsing, add negative caching, fix spinner flicker - #48826
Conversation
…hing; fix spinner flicker 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
|
/matt |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
Improves forecast performance and terminal UX by optimizing workflow-name resolution, caching no-data results, and coordinating concurrent spinners.
Changes:
- Scans lock files for top-level workflow names instead of fully parsing YAML.
- Adds versioned negative caching for runs without AIC data.
- Suppresses concurrent spinner animations globally.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/resolve.go |
Adds targeted lock-file name extraction. |
pkg/workflow/resolve_test.go |
Tests realistic and unquoted names. |
pkg/console/spinner.go |
Adds global spinner coordination. |
pkg/console/spinner_test.go |
Tests concurrent spinner suppression. |
pkg/cli/forecast_compute.go |
Writes negative cache entries. |
pkg/cli/forecast_cache.go |
Implements no-data cache markers. |
pkg/cli/forecast_cache_test.go |
Tests negative caching and invalidation. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
pkg/cli/forecast_compute.go:320
- An analysis error does not establish that the artifact has no AIC data. Malformed/truncated JSONL, a scanner error, or a temporary filesystem read error all return a non-nil
err; writingNoDatahere prevents every later forecast from retrying the already-downloaded artifact until the CLI version changes. Return without negative-caching whenerr != nil, and reserve the marker for a successful analysis with nil/non-positive data.
// 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)
- Files reviewed: 7/7 changed files
- Comments generated: 3
- Review effort level: Medium
| // Suppressed spinner never rendered; release the slot and print the message. | ||
| releaseActiveSpinner(s) | ||
| fmt.Fprintf(s.out, "%s\n", msg) |
| // Negative-cache this completed run so future forecasts don't re-list its | ||
| // (nonexistent) artifacts over the network on every invocation. | ||
| saveForecastNoDataCache(dir, runID) |
| 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. |
|
@copilot fix lint go https://github.com/github/gh-aw/actions/runs/30436976980/job/90526904494?pr=48826 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on two issues.
📋 Key Themes & Highlights
Issues
-
Negative-cache on transient errors (
forecast_compute.go:316): the combinederr != nil || ...guard callssaveForecastNoDataCacheeven whenforecastAnalyzeTokenUsagereturns an error, permanently suppressing retries for a transient failure. Theerr != nilcase should skip the negative-cache write, mirroring the already-correct handling ofcontext.Canceled. -
Test isolation (
spinner_test.go:293):TestSpinnerGlobalCoordinationSuppressesConcurrentleavesglobalSpinnerActiveset (or cleared) depending on execution path, making other spinner tests order-sensitive. Existing tests likeTestNewSpinnerstart real spinners against the same global. Addt.Cleanupto reset the global. Also,first.Stop()is called both explicitly and viadefer— remove one.
Positive Highlights
- ✅ Excellent perf impact: full YAML parse → single-line scan is a clean deep-module improvement.
- ✅ Negative-cache design is sound — version-mismatch invalidation and transient vs. permanent distinction are well thought-out.
- ✅ Global spinner coordination is a correct and minimal fix; suppressed spinners stay lifecycle-safe.
- ✅ Test coverage for all three new behaviours is solid.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 67 AIC · ⌖ 4.94 AIC · ⊞ 6.7K
Comment /matt to run again
…Artifacts over-caching Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in commit 9fc2b99:
|
|
🎉 This pull request is included in a new release. Release: |
Overview
Performance and correctness fixes across forecast caching, workflow lock-file name resolution, and terminal spinner rendering.
Key changes
pkg/cli/forecast_cache.go,forecast_compute.go): adds aNoDatamarker to the forecast AIC cache so runs with no usage artifact / no AIC data are recorded as a cache hit (returning 0), avoiding repeated network lookups on every forecast invocation. A newerrNoMatchingArtifactsentinel (distinct fromErrNoArtifacts) ensures negative caching only applies to permanent no-data conditions — transient failures (context cancellation, download errors) are never cached.pkg/workflow/resolve.go):GetAllWorkflows()no longer fully reads and YAML-parses every generated.lock.ymlfile (which can be hundreds of KB) just to extract the display name. A newextractLockFileWorkflowNamescans line-by-line for the first column-0name:key (skipping indented nested job/stepname:keys) and unmarshals only that line, preserving YAML scalar quoting/escaping semantics.pkg/console/spinner.go): introduces global single-active-spinner coordination (claimActiveSpinner/releaseActiveSpinner) so only one spinner renders to the terminal at a time. Concurrent spinners (e.g. an outer spinner plus parallel per-run download spinners) are markedsuppressed— they track lifecycle state but don't animate — eliminating flicker from overlapping carriage-return/clear-line writes. Also fixes a race inStopWithMessage.Testing
forecast_cache_test.go); errNoMatchingArtifact usage (forecast_test.go); realistic lock-file shape and unquoted-name extraction (resolve_test.go); concurrent spinner suppression/claim/release lifecycle (spinner_test.go).StopWithMessageflicker, and over-eagerErrNoArtifactscaching found during review.No breaking changes.