From 53ad88467d35875b0bb6e396447403bbd7af0f11 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 19 May 2026 00:38:06 +0000 Subject: [PATCH 1/4] Initial plan From 43ee16f2ffa5a4385f4a15318b3e276ffe1c7130 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 19 May 2026 00:54:32 +0000 Subject: [PATCH 2/4] chore: plan once-loader cache refactor Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .github/workflows/mcp-inspector.lock.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/mcp-inspector.lock.yml b/.github/workflows/mcp-inspector.lock.yml index a6c476a93e1..ad1d4a64f61 100644 --- a/.github/workflows/mcp-inspector.lock.yml +++ b/.github/workflows/mcp-inspector.lock.yml @@ -996,7 +996,7 @@ jobs: "url": "https://mcp.datadoghq.com/api/unstable/mcp-server/mcp?toolsets=core", "headers": { "DD_API_KEY": "\${DD_API_KEY}", - "DD_APPLICATION_KEY": "\${DD_APP_KEY}", + "DD_APPLICATION_KEY": "\${DD_APPLICATION_KEY}", "DD_SITE": "\${DD_SITE}" }, "tools": [ From 08c58766e93dd5cecdef693470c92482d5babfa3 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 19 May 2026 00:57:03 +0000 Subject: [PATCH 3/4] refactor shared current-repo once cache Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/repo.go | 24 +--- pkg/cli/repo_test_helpers_test.go | 6 +- pkg/syncutil/onceloader.go | 36 ++++++ pkg/syncutil/onceloader_test.go | 119 ++++++++++++++++++ .../repository_features_validation.go | 24 +--- 5 files changed, 162 insertions(+), 47 deletions(-) create mode 100644 pkg/syncutil/onceloader.go create mode 100644 pkg/syncutil/onceloader_test.go diff --git a/pkg/cli/repo.go b/pkg/cli/repo.go index 5a9b97fba85..de4b6a3a449 100644 --- a/pkg/cli/repo.go +++ b/pkg/cli/repo.go @@ -4,26 +4,16 @@ import ( "fmt" "os/exec" "strings" - "sync" "github.com/github/gh-aw/pkg/logger" + "github.com/github/gh-aw/pkg/syncutil" "github.com/github/gh-aw/pkg/workflow" ) var repoLog = logger.New("cli:repo") -// repoSlugCacheState holds the cached repository slug and protects it with a mutex. -// Using a mutex-guarded struct instead of sync.Once avoids the data race that arises -// when resetting sync.Once via struct assignment (= sync.Once{}) after first use. -type repoSlugCacheState struct { - mu sync.Mutex - result string - err error - done bool -} - // Global cache for current repository info -var currentRepoSlugCache repoSlugCacheState +var currentRepoSlugCache syncutil.OnceLoader[string] // getCurrentRepoSlugUncached gets the current repository slug (owner/repo) using gh CLI (uncached) // Falls back to git remote parsing if gh CLI is not available @@ -91,15 +81,7 @@ func getCurrentRepoSlugUncached() (string, error) { // GetCurrentRepoSlug gets the current repository slug with caching. // This is the recommended function to use for repository access across the codebase. func GetCurrentRepoSlug() (string, error) { - result, err := func() (string, error) { - currentRepoSlugCache.mu.Lock() - defer currentRepoSlugCache.mu.Unlock() - if !currentRepoSlugCache.done { - currentRepoSlugCache.result, currentRepoSlugCache.err = getCurrentRepoSlugUncached() - currentRepoSlugCache.done = true - } - return currentRepoSlugCache.result, currentRepoSlugCache.err - }() + result, err := currentRepoSlugCache.Get(getCurrentRepoSlugUncached) if err != nil { return "", err diff --git a/pkg/cli/repo_test_helpers_test.go b/pkg/cli/repo_test_helpers_test.go index 8658658d673..0cc2cb62e81 100644 --- a/pkg/cli/repo_test_helpers_test.go +++ b/pkg/cli/repo_test_helpers_test.go @@ -5,9 +5,5 @@ package cli // ClearCurrentRepoSlugCache clears the current repository slug cache. // This is useful for testing when repository context might have changed. func ClearCurrentRepoSlugCache() { - currentRepoSlugCache.mu.Lock() - defer currentRepoSlugCache.mu.Unlock() - currentRepoSlugCache.result = "" - currentRepoSlugCache.err = nil - currentRepoSlugCache.done = false + currentRepoSlugCache.Reset() } diff --git a/pkg/syncutil/onceloader.go b/pkg/syncutil/onceloader.go new file mode 100644 index 00000000000..5d75dc0709a --- /dev/null +++ b/pkg/syncutil/onceloader.go @@ -0,0 +1,36 @@ +package syncutil + +import "sync" + +// OnceLoader caches the result of a fallible, expensive one-shot fetch. +// Safe for concurrent use; loader is invoked at most once. +type OnceLoader[T any] struct { + mu sync.Mutex + result T + err error + done bool +} + +// Get returns the cached result, invoking loader exactly once. +func (o *OnceLoader[T]) Get(loader func() (T, error)) (T, error) { + o.mu.Lock() + defer o.mu.Unlock() + + if !o.done { + o.result, o.err = loader() + o.done = true + } + + return o.result, o.err +} + +// Reset clears cached state. +func (o *OnceLoader[T]) Reset() { + o.mu.Lock() + defer o.mu.Unlock() + + var zero T + o.result = zero + o.err = nil + o.done = false +} diff --git a/pkg/syncutil/onceloader_test.go b/pkg/syncutil/onceloader_test.go new file mode 100644 index 00000000000..675350b23ed --- /dev/null +++ b/pkg/syncutil/onceloader_test.go @@ -0,0 +1,119 @@ +package syncutil + +import ( + "errors" + "fmt" + "sync" + "sync/atomic" + "testing" +) + +func TestOnceLoaderGetCachesSuccess(t *testing.T) { + var loader OnceLoader[string] + var calls atomic.Int32 + + load := func() (string, error) { + calls.Add(1) + return "ok", nil + } + + got1, err1 := loader.Get(load) + got2, err2 := loader.Get(load) + + if err1 != nil || err2 != nil { + t.Fatalf("expected nil errors, got err1=%v err2=%v", err1, err2) + } + if got1 != "ok" || got2 != "ok" { + t.Fatalf("expected cached value 'ok', got %q and %q", got1, got2) + } + if calls.Load() != 1 { + t.Fatalf("expected loader to be called once, got %d", calls.Load()) + } +} + +func TestOnceLoaderGetCachesError(t *testing.T) { + var loader OnceLoader[string] + var calls atomic.Int32 + expectedErr := errors.New("boom") + + load := func() (string, error) { + calls.Add(1) + return "", expectedErr + } + + got1, err1 := loader.Get(load) + got2, err2 := loader.Get(load) + + if got1 != "" || got2 != "" { + t.Fatalf("expected empty cached values, got %q and %q", got1, got2) + } + if !errors.Is(err1, expectedErr) || !errors.Is(err2, expectedErr) { + t.Fatalf("expected cached errors to wrap %v, got err1=%v err2=%v", expectedErr, err1, err2) + } + if calls.Load() != 1 { + t.Fatalf("expected loader to be called once, got %d", calls.Load()) + } +} + +func TestOnceLoaderGetConcurrentSingleInvoke(t *testing.T) { + var loader OnceLoader[string] + var calls atomic.Int32 + const workers = 50 + + load := func() (string, error) { + calls.Add(1) + return "value", nil + } + + var wg sync.WaitGroup + wg.Add(workers) + for range workers { + go func() { + defer wg.Done() + got, err := loader.Get(load) + if err != nil { + t.Errorf("expected nil error, got %v", err) + return + } + if got != "value" { + t.Errorf("expected value, got %q", got) + } + }() + } + wg.Wait() + + if calls.Load() != 1 { + t.Fatalf("expected loader to be called once under concurrency, got %d", calls.Load()) + } +} + +func TestOnceLoaderReset(t *testing.T) { + var loader OnceLoader[string] + var calls atomic.Int32 + + load := func() (string, error) { + n := calls.Add(1) + return fmt.Sprintf("v%d", n), nil + } + + got1, err1 := loader.Get(load) + if err1 != nil { + t.Fatalf("unexpected error: %v", err1) + } + if got1 != "v1" { + t.Fatalf("expected first value v1, got %q", got1) + } + + loader.Reset() + + got2, err2 := loader.Get(load) + if err2 != nil { + t.Fatalf("unexpected error after reset: %v", err2) + } + if got2 != "v2" { + t.Fatalf("expected second value v2 after reset, got %q", got2) + } + if calls.Load() != 2 { + t.Fatalf("expected loader to run twice with reset, got %d", calls.Load()) + } +} diff --git a/pkg/workflow/repository_features_validation.go b/pkg/workflow/repository_features_validation.go index 373097cb979..3d0002ec3cc 100644 --- a/pkg/workflow/repository_features_validation.go +++ b/pkg/workflow/repository_features_validation.go @@ -49,6 +49,7 @@ import ( "github.com/cli/go-gh/v2/pkg/api" "github.com/cli/go-gh/v2/pkg/repository" "github.com/github/gh-aw/pkg/console" + "github.com/github/gh-aw/pkg/syncutil" ) var repositoryFeaturesLog = newValidationLogger("repository_features") @@ -59,22 +60,11 @@ type RepositoryFeatures struct { HasIssues bool } -// currentRepositoryCacheState holds the cached current repository and protects it -// with a mutex. Using a mutex-guarded struct instead of sync.Once avoids the data -// race that arises when resetting sync.Once via struct assignment (= sync.Once{}) -// after first use. -type currentRepositoryCacheState struct { - mu sync.Mutex - result string - err error - done bool -} - // Global cache for repository features and current repository info var ( repositoryFeaturesCache = sync.Map{} // sync.Map is thread-safe and efficient for read-heavy workloads repositoryFeaturesLoggedCache = sync.Map{} // Tracks which repositories have had their success messages logged - currentRepositoryCache currentRepositoryCacheState + currentRepositoryCache syncutil.OnceLoader[string] ) // validateRepositoryFeatures validates that required repository features are enabled @@ -157,15 +147,7 @@ func (c *Compiler) validateRepositoryFeatures(workflowData *WorkflowData) error // getCurrentRepository gets the current repository from git context (with caching) func getCurrentRepository() (string, error) { - result, err := func() (string, error) { - currentRepositoryCache.mu.Lock() - defer currentRepositoryCache.mu.Unlock() - if !currentRepositoryCache.done { - currentRepositoryCache.result, currentRepositoryCache.err = getCurrentRepositoryUncached() - currentRepositoryCache.done = true - } - return currentRepositoryCache.result, currentRepositoryCache.err - }() + result, err := currentRepositoryCache.Get(getCurrentRepositoryUncached) if err != nil { return "", err From 3371d8d81ed91e22d1d4b55a0e8daa7abb12954d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 19 May 2026 02:31:18 +0000 Subject: [PATCH 4/4] revert unrelated mcp inspector lock change Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .github/workflows/mcp-inspector.lock.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/mcp-inspector.lock.yml b/.github/workflows/mcp-inspector.lock.yml index ad1d4a64f61..a6c476a93e1 100644 --- a/.github/workflows/mcp-inspector.lock.yml +++ b/.github/workflows/mcp-inspector.lock.yml @@ -996,7 +996,7 @@ jobs: "url": "https://mcp.datadoghq.com/api/unstable/mcp-server/mcp?toolsets=core", "headers": { "DD_API_KEY": "\${DD_API_KEY}", - "DD_APPLICATION_KEY": "\${DD_APPLICATION_KEY}", + "DD_APPLICATION_KEY": "\${DD_APP_KEY}", "DD_SITE": "\${DD_SITE}" }, "tools": [