diff --git a/pkg/parser/remote_list_files.go b/pkg/parser/remote_list_files.go index e62ff8e6d0e..c44a4131f6a 100644 --- a/pkg/parser/remote_list_files.go +++ b/pkg/parser/remote_list_files.go @@ -33,10 +33,21 @@ var gitListCloneCache = struct { var gitListCloneGroup singleflight.Group -func getOrCreateListRepoClone(ctx context.Context, owner, repo, ref, host string) (string, error) { +// listRepoCloneConfig holds the fully-resolved identity of a shallow clone used +// for directory listing. owner and repo are included so callers can pass the +// entire struct without re-expanding individual fields. +type listRepoCloneConfig struct { + owner string + repo string + ref string + repoURL string + cacheKey string +} + +func resolveListRepoCloneConfig(owner, repo, ref, host string) (listRepoCloneConfig, error) { ref = strings.TrimSpace(ref) if ref == "" { - return "", errors.New("git fallback requires a non-empty ref") + return listRepoCloneConfig{}, errors.New("git fallback requires a non-empty ref") } githubHost := GetGitHubHostForRepo(owner, repo) @@ -45,55 +56,103 @@ func getOrCreateListRepoClone(ctx context.Context, owner, repo, ref, host string } repoURL := fmt.Sprintf("%s/%s/%s.git", githubHost, owner, repo) cacheKey := fmt.Sprintf("%s|%s|%s|%s", githubHost, owner, repo, ref) + return listRepoCloneConfig{ + owner: owner, + repo: repo, + ref: ref, + repoURL: repoURL, + cacheKey: cacheKey, + }, nil +} - if cloneDir, found := func() (string, bool) { - gitListCloneCache.mu.Lock() - defer gitListCloneCache.mu.Unlock() - if cloneDir, ok := gitListCloneCache.dirs[cacheKey]; ok { - if stat, err := os.Stat(filepath.Join(cloneDir, ".git")); err == nil && stat.IsDir() { - return cloneDir, true - } - delete(gitListCloneCache.dirs, cacheKey) - } +// readCloneFromCache returns the cached clone directory for cacheKey under lock. +func readCloneFromCache(cacheKey string) (string, bool) { + gitListCloneCache.mu.Lock() + defer gitListCloneCache.mu.Unlock() + cloneDir, ok := gitListCloneCache.dirs[cacheKey] + return cloneDir, ok +} + +// evictCloneFromCache removes the cache entry for cacheKey only if it still maps to +// cloneDir, preventing incorrect eviction of a fresh entry written by another goroutine. +func evictCloneFromCache(cacheKey, cloneDir string) { + gitListCloneCache.mu.Lock() + defer gitListCloneCache.mu.Unlock() + if gitListCloneCache.dirs[cacheKey] == cloneDir { + delete(gitListCloneCache.dirs, cacheKey) + } +} + +// getCachedListRepoClone reads the cached clone path without holding the mutex during +// filesystem I/O to avoid serializing concurrent callers. If the entry is stale it is +// evicted via evictCloneFromCache with a path-equality guard. +// Do not call from code that already holds gitListCloneCache.mu. +func getCachedListRepoClone(cacheKey string) (string, bool) { + cloneDir, ok := readCloneFromCache(cacheKey) + if !ok { return "", false - }(); found { - return cloneDir, nil } + if stat, err := os.Stat(filepath.Join(cloneDir, ".git")); err == nil && stat.IsDir() { + return cloneDir, true + } + // Entry appears stale — evict under lock, but only if it still maps to the same + // path we checked above. A concurrent goroutine may have replaced the entry with a + // fresh clone in the window between the stat and this lock acquisition. + evictCloneFromCache(cacheKey, cloneDir) + return "", false +} - cloneDir, err, _ := gitListCloneGroup.Do(cacheKey, func() (any, error) { - if cloneDir, found := func() (string, bool) { - gitListCloneCache.mu.Lock() - defer gitListCloneCache.mu.Unlock() - if cloneDir, ok := gitListCloneCache.dirs[cacheKey]; ok { - if stat, err := os.Stat(filepath.Join(cloneDir, ".git")); err == nil && stat.IsDir() { - return cloneDir, true - } - delete(gitListCloneCache.dirs, cacheKey) - } - return "", false - }(); found { - return cloneDir, nil - } +// cloneAndCacheListRepoClone performs the clone and writes the result to the cache. +// It must only be called from within a singleflight.Group.Do callback to prevent +// concurrent duplicate clones for the same cache key. The final cache write is +// performed under gitListCloneCache.mu with a defensive check: if another goroutine +// has already populated the entry (e.g. due to incorrect direct invocation), the +// redundant tmpDir is discarded and the existing entry is returned. +// Do not call from code that already holds gitListCloneCache.mu. +func cloneAndCacheListRepoClone(ctx context.Context, cfg listRepoCloneConfig) (string, error) { + tmpDir, err := os.MkdirTemp("", "gh-aw-list-*") + if err != nil { + return "", fmt.Errorf("failed to create temp directory: %w", err) + } - tmpDir, err := os.MkdirTemp("", "gh-aw-list-*") - if err != nil { - return "", fmt.Errorf("failed to create temp directory: %w", err) + cloneCmd := exec.CommandContext(ctx, "git", "clone", "--depth", "1", "--branch", cfg.ref, "--single-branch", "--filter=blob:none", "--no-checkout", cfg.repoURL, tmpDir) + cloneOutput, err := cloneCmd.CombinedOutput() + if err != nil { + if cleanupErr := os.RemoveAll(tmpDir); cleanupErr != nil { + remoteLog.Printf("Failed to clean up temp directory %q: %v", tmpDir, cleanupErr) } + remoteLog.Printf("Failed to clone repository: %s", string(cloneOutput)) + return "", fmt.Errorf("failed to clone repository for %s/%s@%s: %w", cfg.owner, cfg.repo, cfg.ref, err) + } - cloneCmd := exec.CommandContext(ctx, "git", "clone", "--depth", "1", "--branch", ref, "--single-branch", "--filter=blob:none", "--no-checkout", repoURL, tmpDir) - cloneOutput, err := cloneCmd.CombinedOutput() - if err != nil { - if cleanupErr := os.RemoveAll(tmpDir); cleanupErr != nil { - remoteLog.Printf("Failed to clean up temp directory %q: %v", tmpDir, cleanupErr) - } - remoteLog.Printf("Failed to clone repository: %s", string(cloneOutput)) - return "", fmt.Errorf("failed to clone repository for %s/%s@%s: %w", owner, repo, ref, err) + gitListCloneCache.mu.Lock() + defer gitListCloneCache.mu.Unlock() + if existing, ok := gitListCloneCache.dirs[cfg.cacheKey]; ok { + // Another goroutine finished first; discard our redundant clone. + if cleanupErr := os.RemoveAll(tmpDir); cleanupErr != nil { + remoteLog.Printf("Failed to clean up redundant temp directory %q: %v", tmpDir, cleanupErr) } + return existing, nil + } + gitListCloneCache.dirs[cfg.cacheKey] = tmpDir + return tmpDir, nil +} + +func getOrCreateListRepoClone(ctx context.Context, owner, repo, ref, host string) (string, error) { + config, err := resolveListRepoCloneConfig(owner, repo, ref, host) + if err != nil { + return "", err + } - gitListCloneCache.mu.Lock() - gitListCloneCache.dirs[cacheKey] = tmpDir - gitListCloneCache.mu.Unlock() - return tmpDir, nil + if cloneDir, found := getCachedListRepoClone(config.cacheKey); found { + return cloneDir, nil + } + + cloneDir, err, _ := gitListCloneGroup.Do(config.cacheKey, func() (any, error) { + if cloneDir, found := getCachedListRepoClone(config.cacheKey); found { + return cloneDir, nil + } + return cloneAndCacheListRepoClone(ctx, config) }) if err != nil { return "", err diff --git a/pkg/parser/remote_list_files_test.go b/pkg/parser/remote_list_files_test.go new file mode 100644 index 00000000000..ecd415e3951 --- /dev/null +++ b/pkg/parser/remote_list_files_test.go @@ -0,0 +1,132 @@ +//go:build !integration + +package parser + +import ( + "strings" + "testing" +) + +func TestResolveListRepoCloneConfig(t *testing.T) { + tests := []struct { + name string + owner string + repo string + ref string + host string + wantErr bool + checkFn func(t *testing.T, cfg listRepoCloneConfig) + }{ + { + name: "empty ref returns error", + owner: "o", + repo: "r", + ref: "", + wantErr: true, + }, + { + name: "whitespace-only ref returns error", + owner: "o", + repo: "r", + ref: " ", + wantErr: true, + }, + { + name: "host override used in repoURL and cacheKey", + owner: "o", + repo: "r", + ref: "main", + host: "ghes.example.com", + checkFn: func(t *testing.T, cfg listRepoCloneConfig) { + t.Helper() + if !strings.Contains(cfg.repoURL, "ghes.example.com") { + t.Errorf("expected ghes.example.com in repoURL, got %q", cfg.repoURL) + } + if !strings.Contains(cfg.cacheKey, "ghes.example.com") { + t.Errorf("expected ghes.example.com in cacheKey, got %q", cfg.cacheKey) + } + }, + }, + { + name: "cache key contains all identity fields", + owner: "myowner", + repo: "myrepo", + ref: "v1.2.3", + host: "ghes.example.com", + checkFn: func(t *testing.T, cfg listRepoCloneConfig) { + t.Helper() + for _, part := range []string{"myowner", "myrepo", "v1.2.3"} { + if !strings.Contains(cfg.cacheKey, part) { + t.Errorf("missing %q in cacheKey %q", part, cfg.cacheKey) + } + } + }, + }, + { + name: "owner and repo are set on config", + owner: "myowner", + repo: "myrepo", + ref: "main", + host: "ghes.example.com", + checkFn: func(t *testing.T, cfg listRepoCloneConfig) { + t.Helper() + if cfg.owner != "myowner" { + t.Errorf("owner = %q, want %q", cfg.owner, "myowner") + } + if cfg.repo != "myrepo" { + t.Errorf("repo = %q, want %q", cfg.repo, "myrepo") + } + }, + }, + { + name: "ref is trimmed of surrounding whitespace", + owner: "o", + repo: "r", + ref: " main ", + host: "ghes.example.com", + checkFn: func(t *testing.T, cfg listRepoCloneConfig) { + t.Helper() + if cfg.ref != "main" { + t.Errorf("ref = %q, want %q", cfg.ref, "main") + } + }, + }, + { + name: "public github org uses github.com host", + owner: "github", + repo: "myrepo", + ref: "main", + host: "", + checkFn: func(t *testing.T, cfg listRepoCloneConfig) { + t.Helper() + if !strings.Contains(cfg.repoURL, "github.com") { + t.Errorf("expected github.com in repoURL for public org, got %q", cfg.repoURL) + } + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Isolate environment so GetGitHubHostForRepo uses a predictable host. + t.Setenv("GITHUB_SERVER_URL", "") + t.Setenv("GITHUB_ENTERPRISE_HOST", "") + t.Setenv("GITHUB_HOST", "") + t.Setenv("GH_HOST", "") + + cfg, err := resolveListRepoCloneConfig(tt.owner, tt.repo, tt.ref, tt.host) + if tt.wantErr { + if err == nil { + t.Fatalf("resolveListRepoCloneConfig() error = nil, want error") + } + return + } + if err != nil { + t.Fatalf("resolveListRepoCloneConfig() unexpected error: %v", err) + } + if tt.checkFn != nil { + tt.checkFn(t, cfg) + } + }) + } +}