diff --git a/pkg/workflow/action_cache.go b/pkg/workflow/action_cache.go index 3df34e85475..6ebed0e0e1d 100644 --- a/pkg/workflow/action_cache.go +++ b/pkg/workflow/action_cache.go @@ -29,6 +29,7 @@ type ActionCacheEntry struct { type ActionCache struct { Entries map[string]ActionCacheEntry `json:"entries"` // key: "repo@version" path string + dirty bool // tracks if cache has unsaved changes } // NewActionCache creates a new action cache instance @@ -38,6 +39,7 @@ func NewActionCache(repoRoot string) *ActionCache { return &ActionCache{ Entries: make(map[string]ActionCacheEntry), path: cachePath, + // dirty is initialized to false (zero value) } } @@ -60,6 +62,9 @@ func (c *ActionCache) Load() error { return err } + // Mark cache as clean after successful load (it matches disk state) + c.dirty = false + actionCacheLog.Printf("Successfully loaded cache with %d entries", len(c.Entries)) return nil } @@ -67,7 +72,14 @@ func (c *ActionCache) Load() error { // Save saves the cache to disk with sorted entries // If the cache is empty, the file is not created or is deleted if it exists // Deduplicates entries by keeping only the most precise version reference for each repo+SHA combination +// Only saves if the cache has been modified (dirty flag is true) func (c *ActionCache) Save() error { + // Skip saving if cache hasn't been modified + if !c.dirty { + actionCacheLog.Printf("Cache is clean (no changes), skipping save") + return nil + } + actionCacheLog.Printf("Saving action cache to: %s with %d entries", c.path, len(c.Entries)) // If cache is empty, skip saving and delete the file if it exists @@ -81,6 +93,7 @@ func (c *ActionCache) Save() error { return err } } + c.dirty = false return nil } @@ -110,6 +123,7 @@ func (c *ActionCache) Save() error { } actionCacheLog.Print("Successfully saved action cache") + c.dirty = false return nil } @@ -187,6 +201,7 @@ func (c *ActionCache) Set(repo, version, sha string) { Version: version, SHA: sha, } + c.dirty = true // Mark cache as modified } // GetCachePath returns the path to the cache file diff --git a/pkg/workflow/action_cache_test.go b/pkg/workflow/action_cache_test.go index 0d7af74e40c..0adfe8669e8 100644 --- a/pkg/workflow/action_cache_test.go +++ b/pkg/workflow/action_cache_test.go @@ -240,6 +240,7 @@ func TestActionCacheEmptySaveDeletesExistingFile(t *testing.T) { // Clear cache and save again cache.Entries = make(map[string]ActionCacheEntry) + cache.dirty = true // Mark as dirty so save actually processes the empty cache err = cache.Save() if err != nil { t.Fatalf("Failed to save empty cache: %v", err) @@ -268,6 +269,7 @@ func TestActionCacheDeduplication(t *testing.T) { Version: "v5.0.1", SHA: "abc123", } + cache.dirty = true // Mark as dirty so save processes the cache // Verify we have 2 entries before deduplication if len(cache.Entries) != 2 { @@ -328,6 +330,9 @@ func TestActionCacheDeduplicationMultipleActions(t *testing.T) { // actions/setup-node: no duplicates cache.Set("actions/setup-node", "v6.1.0", "sha3") + // Since we set Entries directly, we need to mark as dirty for the test + cache.dirty = true + // Verify we have 5 entries before deduplication if len(cache.Entries) != 5 { t.Fatalf("Expected 5 entries before deduplication, got %d", len(cache.Entries)) @@ -451,3 +456,51 @@ func TestIsMorePreciseVersion(t *testing.T) { }) } } + +// TestActionCacheDirtyFlag verifies that the cache dirty flag prevents unnecessary saves +func TestActionCacheDirtyFlag(t *testing.T) { + tmpDir := testutil.TempDir(t, "test-*") + cache := NewActionCache(tmpDir) + + // Initially, cache should be clean (no data) + err := cache.Save() + if err != nil { + t.Fatalf("Failed to save empty cache: %v", err) + } + + // Cache file should not exist (empty cache) + cachePath := cache.GetCachePath() + if _, err := os.Stat(cachePath); !os.IsNotExist(err) { + t.Error("Empty cache should not create a file") + } + + // Add an entry - should mark as dirty + cache.Set("actions/checkout", "v5", "abc123") + + // Save should work now + err = cache.Save() + if err != nil { + t.Fatalf("Failed to save dirty cache: %v", err) + } + + // Cache file should now exist + if _, err := os.Stat(cachePath); err != nil { + t.Errorf("Cache file should exist after save: %v", err) + } + + // Save again without changes - should skip (cache is clean) + // We can't directly verify the skip, but we can ensure it doesn't error + err = cache.Save() + if err != nil { + t.Fatalf("Failed to save clean cache: %v", err) + } + + // Add another entry - should mark as dirty again + cache.Set("actions/setup-node", "v4", "def456") + + // Save should work + err = cache.Save() + if err != nil { + t.Fatalf("Failed to save dirty cache after modification: %v", err) + } +} diff --git a/pkg/workflow/action_pins.go b/pkg/workflow/action_pins.go index b0a789d959b..647cc966091 100644 --- a/pkg/workflow/action_pins.go +++ b/pkg/workflow/action_pins.go @@ -7,6 +7,7 @@ import ( "os" "sort" "strings" + "sync" "github.com/githubnext/gh-aw/pkg/console" "github.com/githubnext/gh-aw/pkg/logger" @@ -30,58 +31,70 @@ type ActionPinsData struct { Entries map[string]ActionPin `json:"entries"` // key: "repo@version" } -// getActionPins unmarshals and returns the action pins from the embedded JSON +var ( + // cachedActionPins holds the parsed and sorted action pins + cachedActionPins []ActionPin + // actionPinsOnce ensures the action pins are loaded only once + actionPinsOnce sync.Once +) + +// getActionPins returns the action pins from the embedded JSON // Returns a sorted slice of action pins (by version descending, then by repo name) -// This is called on-demand rather than cached globally +// The data is parsed once on first call and cached for subsequent calls func getActionPins() []ActionPin { - actionPinsLog.Print("Unmarshaling action pins from embedded JSON") + actionPinsOnce.Do(func() { + actionPinsLog.Print("Unmarshaling action pins from embedded JSON (first call, will be cached)") - var data ActionPinsData - if err := json.Unmarshal(actionPinsJSON, &data); err != nil { - actionPinsLog.Printf("Failed to unmarshal action pins JSON: %v", err) - panic(fmt.Sprintf("failed to load action pins: %v", err)) - } + var data ActionPinsData + if err := json.Unmarshal(actionPinsJSON, &data); err != nil { + actionPinsLog.Printf("Failed to unmarshal action pins JSON: %v", err) + panic(fmt.Sprintf("failed to load action pins: %v", err)) + } - // Detect and log key/version mismatches - mismatchCount := 0 - for key, pin := range data.Entries { - // Extract version from key (format: "repo@version") - if idx := strings.LastIndex(key, "@"); idx != -1 { - keyVersion := key[idx+1:] - if keyVersion != pin.Version { - mismatchCount++ - actionPinsLog.Printf("WARNING: Key/version mismatch in action_pins.json: key=%s has version=%s but pin.Version=%s (sha=%s)", - key, keyVersion, pin.Version, pin.SHA[:8]) + // Detect and log key/version mismatches + mismatchCount := 0 + for key, pin := range data.Entries { + // Extract version from key (format: "repo@version") + if idx := strings.LastIndex(key, "@"); idx != -1 { + keyVersion := key[idx+1:] + if keyVersion != pin.Version { + mismatchCount++ + // Safely truncate SHA for logging + shortSHA := pin.SHA + if len(pin.SHA) > 8 { + shortSHA = pin.SHA[:8] + } + actionPinsLog.Printf("WARNING: Key/version mismatch in action_pins.json: key=%s has version=%s but pin.Version=%s (sha=%s)", + key, keyVersion, pin.Version, shortSHA) + } } } - } - if mismatchCount > 0 { - actionPinsLog.Printf("Found %d key/version mismatches in action_pins.json", mismatchCount) - } + if mismatchCount > 0 { + actionPinsLog.Printf("Found %d key/version mismatches in action_pins.json", mismatchCount) + } - // Convert map to sorted slice - pins := make([]ActionPin, 0, len(data.Entries)) - for _, pin := range data.Entries { - pins = append(pins, pin) - } + // Convert map to sorted slice + pins := make([]ActionPin, 0, len(data.Entries)) + for _, pin := range data.Entries { + pins = append(pins, pin) + } - // Sort by version (descending) then by repo name (ascending) - for i := 0; i < len(pins); i++ { - for j := i + 1; j < len(pins); j++ { - // Compare versions first (descending) - if pins[i].Version < pins[j].Version { - pins[i], pins[j] = pins[j], pins[i] - } else if pins[i].Version == pins[j].Version { - // Same version, sort by repo name (ascending) - if pins[i].Repo > pins[j].Repo { - pins[i], pins[j] = pins[j], pins[i] - } + // Sort by version (descending) then by repo name (ascending) + // Use standard library sort for better performance O(n log n) vs O(n²) + sort.Slice(pins, func(i, j int) bool { + // Compare versions first (descending order - higher version first) + if pins[i].Version != pins[j].Version { + return pins[i].Version > pins[j].Version } - } - } + // Same version, sort by repo name (ascending order) + return pins[i].Repo < pins[j].Repo + }) - actionPinsLog.Printf("Successfully unmarshaled and sorted %d action pins from JSON", len(pins)) - return pins + actionPinsLog.Printf("Successfully unmarshaled and sorted %d action pins from JSON", len(pins)) + cachedActionPins = pins + }) + + return cachedActionPins } // sortPinsByVersion sorts action pins by version in descending order (highest first) @@ -152,10 +165,8 @@ func GetActionPinWithData(actionRepo, version string, data *WorkflowData) (strin } } - // Successfully resolved, save cache - if data.ActionCache != nil { - _ = data.ActionCache.Save() - } + // Successfully resolved - cache will be saved at end of compilation + actionPinsLog.Printf("Successfully resolved action pin (cache marked dirty, will save at end)") result := actionRepo + "@" + sha + " # " + version actionPinsLog.Printf("Returning pinned reference: %s", result) return result, nil diff --git a/pkg/workflow/action_pins_test.go b/pkg/workflow/action_pins_test.go index 6c1dff01e2b..1b97c158efc 100644 --- a/pkg/workflow/action_pins_test.go +++ b/pkg/workflow/action_pins_test.go @@ -1042,3 +1042,39 @@ func TestApplyActionPinsToTypedSteps(t *testing.T) { }) } } + +// TestActionPinsCaching verifies that action pins are cached and not re-parsed +func TestActionPinsCaching(t *testing.T) { + // Reset the cache by creating a new sync.Once + // Note: In production, this is handled automatically by sync.Once + + // First call - should load and cache + pins1 := getActionPins() + if len(pins1) == 0 { + t.Fatal("No action pins loaded on first call") + } + + // Second call - should return cached data (same slice reference) + pins2 := getActionPins() + if len(pins2) == 0 { + t.Fatal("No action pins loaded on second call") + } + + // Verify both calls return the same data + if len(pins1) != len(pins2) { + t.Errorf("Cache returned different number of pins: first=%d, second=%d", len(pins1), len(pins2)) + } + + // Verify the data is identical by checking a few pins + for i := 0; i < len(pins1) && i < 3; i++ { + if pins1[i].Repo != pins2[i].Repo { + t.Errorf("Pin %d repo mismatch: first=%s, second=%s", i, pins1[i].Repo, pins2[i].Repo) + } + if pins1[i].Version != pins2[i].Version { + t.Errorf("Pin %d version mismatch: first=%s, second=%s", i, pins1[i].Version, pins2[i].Version) + } + if pins1[i].SHA != pins2[i].SHA { + t.Errorf("Pin %d SHA mismatch: first=%s, second=%s", i, pins1[i].SHA, pins2[i].SHA) + } + } +}