Skip to content

Refactor getOrCreateListRepoClone into focused helpers in parser remote file listing #44179

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 8 commits into from
Jul 8, 2026
+233 −42
Merged

Refactor getOrCreateListRepoClone into focused helpers in parser remote file listing #44179

Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a3f1e34
Initial plan
Copilot Jul 8, 2026
8fb3f3f
Refactor parser clone helper to satisfy largefunc lint
Copilot Jul 8, 2026
410fe4a
refactor: extend listRepoCloneConfig with owner/repo, simplify cloneA…
Copilot Jul 8, 2026
b5d8ef4
plan: address remaining mutex/I/O and defensive cache-check threads
Copilot Jul 8, 2026
23c1c2e
fix: release mutex before os.Stat in getCachedListRepoClone; add defe…
Copilot Jul 8, 2026
3f2dee4
fix: guard stale-entry eviction with path equality check; log RemoveA…
Copilot Jul 8, 2026
6129ab3
plan: fix manualmutexunlock lint errors and revert .lock.yml changes
Copilot Jul 8, 2026
b143c2a
fix: extract readCloneFromCache/evictCloneFromCache to satisfy manual…
Copilot Jul 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 57 additions & 43 deletions pkg/parser/remote_list_files.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,16 @@ var gitListCloneCache = struct {

var gitListCloneGroup singleflight.Group

func getOrCreateListRepoClone(ctx context.Context, owner, repo, ref, host string) (string, error) {
type listRepoCloneConfig struct {
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)
Expand All @@ -45,55 +51,63 @@ 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{
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)
func getCachedListRepoClone(cacheKey string) (string, bool) {

@github-actions github-actions Bot Jul 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] The new getCachedListRepoClone and resolveListRepoCloneConfig helpers are now package-private, pure-ish functions that are straightforwardly unit-testable in isolation — but no tests cover them. At minimum, resolveListRepoCloneConfig has edge-case logic (empty ref, host override) that benefits from table-driven tests.

💡 Suggested test skeleton 
func TestResolveListRepoCloneConfig(t *testing.T) {
    tests := []struct {
        name    string
        owner, repo, ref, host string
        wantErr bool
        checkFn func(t *testing.T, cfg listRepoCloneConfig)
    }{
        {name: "empty ref returns error", ref: "  ", wantErr: true},
        {name: "host override used in repoURL", owner: "o", repo: "r", ref: "main", host: "(ghes.example.com/redacted)", checkFn: func(t *testing.T, cfg listRepoCloneConfig) {
            if !strings.Contains(cfg.repoURL, "ghes.example.com") {
                t.Errorf("expected ghes host in repoURL, got %s", cfg.repoURL)
            }
        }},
        {name: "cache key contains all identity fields", owner: "o", repo: "r", ref: "main", checkFn: func(t *testing.T, cfg listRepoCloneConfig) {
            for _, part := range []string{"o", "r", "main"} {
                if !strings.Contains(cfg.cacheKey, part) {
                    t.Errorf("missing %q in cacheKey %q", part, cfg.cacheKey)
                }
            }
        }},
    }
    // ...
}

@copilot please address this.

Copilot AI Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 410fe4a. Added TestResolveListRepoCloneConfig in remote_list_files_test.go with table-driven cases covering: empty ref error, whitespace-only ref error, host override in repoURL/cacheKey, cache key contains all identity fields, owner/repo fields on config, ref trimming, and public-org host selection.

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() {

@github-actions github-actions Bot Jul 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filesystem I/O () is called while holding the global cache mutex, serializing all concurrent cache lookups.

💡 Details and suggested fix

In getCachedListRepoClone, the mutex is held for the entire duration of the function including the os.Stat call on line 66. In the original IIFE this was identical, but extracting it into a standalone helper now silently bakes this behavior into every future caller.

Under contention (multiple goroutines needing a cache-miss check simultaneously), every goroutine must wait in line behind the goroutine doing the os.Stat syscall. If the cloneDir is on a slow or remote filesystem, this serializes all callers.

Suggested fix — copy the path while locked, then stat without the lock:

func getCachedListRepoClone(cacheKey string) (string, bool) {
    gitListCloneCache.mu.Lock()
    cloneDir, ok := gitListCloneCache.dirs[cacheKey]
    gitListCloneCache.mu.Unlock()

    if !ok {
        return "", false
    }
    if stat, err := os.Stat(filepath.Join(cloneDir, ".git")); err == nil && stat.IsDir() {
        return cloneDir, true
    }
    // Entry is stale — evict under lock.
    gitListCloneCache.mu.Lock()
    delete(gitListCloneCache.dirs, cacheKey)
    gitListCloneCache.mu.Unlock()
    return "", false
}

Note: there is a benign TOCTOU window between the stat and the delete — a concurrent goroutine could re-add a fresh entry between those two steps — but singleflight ensures only one goroutine will actually clone, so the stale-eviction path is already racy in the original code.

Copilot AI Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 23c1c2e. getCachedListRepoClone now reads the cached path under a brief lock acquisition, releases the lock before os.Stat, and re-acquires only for the stale-entry eviction — matching the suggested pattern exactly.

return cloneDir, true
}
return "", false
}(); found {
return cloneDir, nil
delete(gitListCloneCache.dirs, cacheKey)
}
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
}
func cloneAndCacheListRepoClone(ctx context.Context, owner, repo, ref, repoURL, cacheKey string) (string, error) {

@github-actions github-actions Bot Jul 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/codebase-design] cloneAndCacheListRepoClone takes owner/repo separately alongside config-derived ref/repoURL/cacheKey, creating a mixed-extraction call site at line 110 (ctx, owner, repo, config.ref, config.repoURL, config.cacheKey). Adding owner and repo to listRepoCloneConfig would complete the struct as a full clone-identity value and reduce this signature to (ctx, cfg).

💡 Suggested refactor

Extend the struct:

type listRepoCloneConfig struct {
	owner    string
	repo     string
	ref      string
	repoURL  string
	cacheKey string
}

Simplify the function:

func cloneAndCacheListRepoClone(ctx context.Context, cfg listRepoCloneConfig) (string, error) {
    // use cfg.owner, cfg.repo, cfg.ref, cfg.repoURL, cfg.cacheKey
}
// call site becomes:
return cloneAndCacheListRepoClone(ctx, config)

This makes listRepoCloneConfig a complete identity type and avoids unpacking the struct at the call site.

@copilot please address this.

Copilot AI Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 410fe4a. owner and repo are now fields on listRepoCloneConfig; cloneAndCacheListRepoClone takes (ctx context.Context, cfg listRepoCloneConfig) and uses cfg.* throughout.

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", 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)
}

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()
Comment thread
pelikhan marked this conversation as resolved.
defer gitListCloneCache.mu.Unlock()
gitListCloneCache.dirs[cacheKey] = tmpDir

@github-actions github-actions Bot Jul 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cloneAndCacheListRepoClone silently assumes it will only ever be called inside a singleflight.Do guard, but has no documentation or enforcement of that precondition — if called directly, a concurrent duplicate invocation would orphan a tmpDir with no cleanup.

💡 Details and suggested fix

The function creates a tmpDir, clones into it, then unconditionally writes gitListCloneCache.dirs[cacheKey] = tmpDir (line 92). It does not re-check whether another caller has already inserted a fresh entry. The singleflight in getOrCreateListRepoClone serializes callers under the same key today — but this is an implicit contract that the extracted function does not document.

If cloneAndCacheListRepoClone is ever called directly (e.g. in a test or a second call-site added later), two goroutines with the same cacheKey would each clone successfully, the last writer would win, and the first goroutine's tmpDir leaks for the process lifetime. The cleanup on clone failure (lines 83-85) does not handle the success path.

Suggested fix — add a defensive cache check before writing:

gitListCloneCache.mu.Lock()
defer gitListCloneCache.mu.Unlock()
if existing, ok := gitListCloneCache.dirs[cacheKey]; ok {
    // Another goroutine finished first; discard our redundant clone.
    _ = os.RemoveAll(tmpDir)
    return existing, nil
}
gitListCloneCache.dirs[cacheKey] = tmpDir
return tmpDir, nil

At minimum, add a doc-comment stating the singleflight precondition so callers cannot accidentally bypass it.

Copilot AI Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 23c1c2e. cloneAndCacheListRepoClone now performs a defensive check under the cache lock before writing: if another goroutine already populated the entry, the redundant tmpDir is cleaned up via os.RemoveAll and the existing entry is returned. The doc comment also explicitly states the singleflight precondition.

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, owner, repo, config.ref, config.repoURL, config.cacheKey)
})
if err != nil {
return "", err
Expand Down
Loading