diff --git a/pkg/actionpins/actionpins_internal_test.go b/pkg/actionpins/actionpins_internal_test.go index 86e51b9628c..bdd400953f7 100644 --- a/pkg/actionpins/actionpins_internal_test.go +++ b/pkg/actionpins/actionpins_internal_test.go @@ -420,10 +420,8 @@ func TestGetContainerPin_ReturnsPinnedImage(t *testing.T) { } func TestGetContainerPin_MCPGatewayVersionsArePinned(t *testing.T) { - getActionPins() - var mcpgImages []string - for image := range cachedContainerPins { + for image := range getCachedActionPins().containers { if strings.HasPrefix(image, "ghcr.io/github/gh-aw-mcpg:") { mcpgImages = append(mcpgImages, image) } @@ -467,6 +465,16 @@ func TestGetActionPins_CacheCorrectnessOnRepeatedCalls(t *testing.T) { assert.Equal(t, first, second, "Expected repeated calls to getActionPins() to return equal data (cache correctness)") } +func TestGetCachedActionPins_InitializesCache(t *testing.T) { + cache := getCachedActionPins() + + require.NotNil(t, cache, "Expected cache accessor to return initialized data") + assert.NotEmpty(t, cache.pins, "Expected cached action pins") + assert.NotNil(t, cache.byRepo, "Expected cached action pins by repository") + assert.NotNil(t, cache.containers, "Expected cached container pins") + assert.Same(t, cache, getCachedActionPins(), "Expected repeated cache access to return the same cache") +} + func TestResolveActionPinDynamically_SkipsForSHAInput(t *testing.T) { t.Parallel() resolver := &countingResolver{} diff --git a/pkg/actionpins/data.go b/pkg/actionpins/data.go index 09a47d24ae1..55d5283fc02 100644 --- a/pkg/actionpins/data.go +++ b/pkg/actionpins/data.go @@ -19,14 +19,22 @@ var actionPinsLog = logger.New("actionpins:actionpins") //go:embed data/action_pins.json var actionPinsJSON []byte +// actionPinsCache bundles the parsed/derived action pin data behind a single +// pointer so the package-level variable holding it is never a bare slice or +// map that gets reassigned in place; the pointer itself is written exactly +// once (guarded by actionPinsOnce) and is treated as read-only thereafter. +type actionPinsCache struct { + pins []ActionPin + byRepo map[string][]ActionPin + containers map[string]ContainerPin +} + var ( - cachedActionPins []ActionPin - cachedActionPinsByRepo map[string][]ActionPin - cachedContainerPins map[string]ContainerPin - actionPinsOnce sync.Once + cachedPins *actionPinsCache + actionPinsOnce sync.Once ) -func getActionPins() []ActionPin { +func getCachedActionPins() *actionPinsCache { actionPinsOnce.Do(func() { actionPinsLog.Print("Unmarshaling action pins from embedded JSON (first call, will be cached)") @@ -42,19 +50,31 @@ func getActionPins() []ActionPin { }) actionPinsLog.Printf("Successfully unmarshaled and sorted %d action pins from JSON", len(pins)) - cachedActionPins = pins - cachedActionPinsByRepo = buildByRepoIndex(pins) - actionPinsLog.Printf("Built per-repo action pin index for %d repos", len(cachedActionPinsByRepo)) + byRepo := buildByRepoIndex(pins) + actionPinsLog.Printf("Built per-repo action pin index for %d repos", len(byRepo)) + + containers := data.Containers + if containers == nil { + containers = make(map[string]ContainerPin) + } + actionPinsLog.Printf("Loaded %d container pins from JSON", len(containers)) - cachedContainerPins = data.Containers - if cachedContainerPins == nil { - cachedContainerPins = make(map[string]ContainerPin) + cachedPins = &actionPinsCache{ + pins: pins, + byRepo: byRepo, + containers: containers, } - actionPinsLog.Printf("Loaded %d container pins from JSON", len(cachedContainerPins)) }) - return cachedActionPins + if cachedPins == nil { + panic("action pins cache was not initialized") + } + return cachedPins +} + +func getActionPins() []ActionPin { + return getCachedActionPins().pins } // loadActionPinsData unmarshals embedded action pin data. @@ -129,8 +149,7 @@ func buildByRepoIndex(pins []ActionPin) map[string][]ActionPin { // GetActionPinsByRepo returns the sorted (version-descending) list of action pins // for the given repository. Returns nil if the repo has no pins. func GetActionPinsByRepo(repo string) []ActionPin { - getActionPins() - return cachedActionPinsByRepo[repo] + return getCachedActionPins().byRepo[repo] } // GetLatestActionPinByRepo returns the latest ActionPin for a given repository, if any. @@ -144,7 +163,6 @@ func GetLatestActionPinByRepo(repo string) (ActionPin, bool) { // GetContainerPin returns a pinned container image by its original image reference. func GetContainerPin(image string) (ContainerPin, bool) { - getActionPins() - pin, ok := cachedContainerPins[image] + pin, ok := getCachedActionPins().containers[image] return pin, ok } diff --git a/pkg/parser/virtual_fs.go b/pkg/parser/virtual_fs.go index 06e51805b05..8dc3d69d9fd 100644 --- a/pkg/parser/virtual_fs.go +++ b/pkg/parser/virtual_fs.go @@ -15,10 +15,15 @@ var virtualFsLog = logger.New("parser:virtual_fs") // builtinVirtualFiles holds embedded built-in files registered at startup. // Keys use the "@builtin:" path prefix (e.g. "@builtin:engines/copilot.md"). -// The map is replaced using copy-on-write during registration and then treated -// as read-only; concurrent reads are safe. +// Registration swaps a pointer to an immutable snapshot rather than assigning +// a map directly. The named snapshot type makes this copy-on-write pattern +// explicit: readers only see fully-populated, read-only snapshots. +type builtinVirtualFileSnapshot struct { + files map[string][]byte +} + var ( - builtinVirtualFiles map[string][]byte + builtinVirtualFiles = &builtinVirtualFileSnapshot{files: map[string][]byte{}} builtinVirtualFilesMu sync.RWMutex ) @@ -33,24 +38,25 @@ func RegisterBuiltinVirtualFile(path string, content []byte) { } builtinVirtualFilesMu.Lock() defer builtinVirtualFilesMu.Unlock() - if existing, ok := builtinVirtualFiles[path]; ok { + current := builtinVirtualFiles.files + if existing, ok := current[path]; ok { if !bytes.Equal(existing, content) { panic(fmt.Sprintf("RegisterBuiltinVirtualFile: path %q already registered with different content", path)) } return // idempotent: same content, no-op } virtualFsLog.Printf("Registering builtin virtual file: %s (%d bytes)", path, len(content)) - next := make(map[string][]byte, len(builtinVirtualFiles)+1) - maps.Copy(next, builtinVirtualFiles) + next := make(map[string][]byte, len(current)+1) + maps.Copy(next, current) next[path] = bytes.Clone(content) - builtinVirtualFiles = next + builtinVirtualFiles = &builtinVirtualFileSnapshot{files: next} } // BuiltinVirtualFileExists returns true if the given path is registered as a builtin virtual file. func BuiltinVirtualFileExists(path string) bool { builtinVirtualFilesMu.RLock() defer builtinVirtualFilesMu.RUnlock() - _, ok := builtinVirtualFiles[path] + _, ok := builtinVirtualFiles.files[path] virtualFsLog.Printf("BuiltinVirtualFileExists: path=%s exists=%t", path, ok) return ok } @@ -116,7 +122,7 @@ const BuiltinPathPrefix = "@builtin:" var readFileFunc = func(path string) ([]byte, error) { builtinVirtualFilesMu.RLock() defer builtinVirtualFilesMu.RUnlock() - content, ok := builtinVirtualFiles[path] + content, ok := builtinVirtualFiles.files[path] if ok { return bytes.Clone(content), nil } diff --git a/pkg/parser/virtual_fs_wasm.go b/pkg/parser/virtual_fs_wasm.go index ee964e7b977..ec48a04b376 100644 --- a/pkg/parser/virtual_fs_wasm.go +++ b/pkg/parser/virtual_fs_wasm.go @@ -38,7 +38,7 @@ func init() { // Check builtin virtual files first (embedded engine .md files etc.) builtinVirtualFilesMu.RLock() defer builtinVirtualFilesMu.RUnlock() - builtinContent, builtinOK := builtinVirtualFiles[path] + builtinContent, builtinOK := builtinVirtualFiles.files[path] if builtinOK { parserLog.Printf("readFileFunc: resolved builtin virtual file: %s", path) return builtinContent, nil