chore: Addressing review feedback on #5989 (#5991)

Author: yhakbar

internal/cache/cache_test.go

@@ -1,11 +1,15 @@
 package cache_test
 
 import (
+	"context"
+	"path/filepath"
 	"testing"
+	"testing/synctest"
 	"time"
 
 	"github.com/gruntwork-io/terragrunt/internal/cache"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestCacheCreation(t *testing.T) {
@@ -52,32 +56,155 @@ func TestExpiringCacheCreation(t *testing.T) {
 func TestExpiringCacheOperation(t *testing.T) {
 	t.Parallel()
 
-	ctx := t.Context()
-	cache := cache.NewExpiringCache[string]("test")
+	synctest.Test(t, func(t *testing.T) {
+		ctx := t.Context()
+		cache := cache.NewExpiringCache[string]("test")
 
-	value, found := cache.Get(ctx, "potato")
+		value, found := cache.Get(ctx, "potato")
 
-	assert.False(t, found)
-	assert.Empty(t, value)
+		assert.False(t, found)
+		assert.Empty(t, value)
 
-	cache.Put(ctx, "potato", "carrot", time.Now().Add(1*time.Second))
-	value, found = cache.Get(ctx, "potato")
+		cache.Put(ctx, "potato", "carrot", time.Now().Add(1*time.Second))
+		value, found = cache.Get(ctx, "potato")
 
-	assert.True(t, found)
-	assert.NotEmpty(t, value)
-	assert.Equal(t, "carrot", value)
+		assert.True(t, found)
+		assert.NotEmpty(t, value)
+		assert.Equal(t, "carrot", value)
+	})
 }
 
 func TestExpiringCacheExpiration(t *testing.T) {
 	t.Parallel()
 
+	synctest.Test(t, func(t *testing.T) {
+		ctx := t.Context()
+		cache := cache.NewExpiringCache[string]("test")
+
+		cache.Put(ctx, "potato", "carrot", time.Now().Add(time.Second))
+
+		// Move the bubble's virtual clock past the expiration so the Get below
+		// observes the entry as expired without any real wallclock wait.
+		time.Sleep(2 * time.Second)
+
+		value, found := cache.Get(ctx, "potato")
+
+		assert.False(t, found)
+		assert.NotEmpty(t, value)
+		assert.Equal(t, "carrot", value)
+	})
+}
+
+func TestContextCache(t *testing.T) {
+	t.Parallel()
+
 	ctx := t.Context()
-	cache := cache.NewExpiringCache[string]("test")
 
-	cache.Put(ctx, "potato", "carrot", time.Now().Add(-1*time.Second))
-	value, found := cache.Get(ctx, "potato")
+	// Missing entry returns a fresh detached instance.
+	c := cache.ContextCache[int](ctx, "not-installed")
+	require.NotNil(t, c)
+	c.Put(ctx, "k", 7)
 
-	assert.False(t, found)
-	assert.NotEmpty(t, value)
-	assert.Equal(t, "carrot", value)
+	// Installed entry round-trips.
+	installed := cache.NewCache[int]("installed")
+	ctxWith := context.WithValue(ctx, cache.RunCmdCacheContextKey, installed)
+
+	got := cache.ContextCache[int](ctxWith, cache.RunCmdCacheContextKey)
+	assert.Same(t, installed, got)
+}
+
+func TestContextWithCacheInstallsBoth(t *testing.T) {
+	t.Parallel()
+
+	ctx := cache.ContextWithCache(t.Context())
+
+	runCmd, ok := ctx.Value(cache.RunCmdCacheContextKey).(*cache.Cache[string])
+	require.True(t, ok)
+	require.NotNil(t, runCmd)
+
+	repoRoots, ok := ctx.Value(cache.RepoRootCacheContextKey).(*cache.RepoRootCache)
+	require.True(t, ok)
+	require.NotNil(t, repoRoots)
+	assert.Equal(t, 0, repoRoots.Len())
+}
+
+func TestRepoRootCacheLookupAndAdd(t *testing.T) {
+	t.Parallel()
+
+	ctx := t.Context()
+	c := cache.NewRepoRootCache("repo")
+
+	// Empty cache misses.
+	_, ok := c.Lookup(ctx, filepath.FromSlash("/a/b"))
+	assert.False(t, ok)
+
+	outer := filepath.FromSlash("/repo")
+	inner := filepath.FromSlash("/repo/sub/nested")
+
+	// Add deepest-first, then a shallower root, to exercise insertion ordering.
+	c.Add(ctx, outer)
+	c.Add(ctx, inner)
+	// Duplicate Add is a no-op.
+	c.Add(ctx, outer)
+	// Empty Add is a no-op.
+	c.Add(ctx, "")
+	assert.Equal(t, 2, c.Len())
+
+	// Adding a shallower-still root exercises the "no insertAt found" branch
+	// where the new root is shorter than every existing one and is appended.
+	shallow := filepath.FromSlash("/r")
+	c.Add(ctx, shallow)
+	assert.Equal(t, 3, c.Len())
+
+	// Exact-path hit returns the matching root.
+	got, ok := c.Lookup(ctx, outer)
+	assert.True(t, ok)
+	assert.Equal(t, outer, got)
+
+	// Descendant of the deeper root prefers it over the shallow one.
+	got, ok = c.Lookup(ctx, filepath.Join(inner, "x"))
+	assert.True(t, ok)
+	assert.Equal(t, inner, got)
+
+	// Sibling that prefix-matches lexically but not on a separator boundary
+	// is not a hit (e.g. /repobar should not match /repo).
+	_, ok = c.Lookup(ctx, filepath.FromSlash("/repobar/x"))
+	assert.False(t, ok)
+
+	// Path outside any cached root misses entirely.
+	_, ok = c.Lookup(ctx, filepath.FromSlash("/elsewhere"))
+	assert.False(t, ok)
+}
+
+func TestRepoRootCacheBeginEndResolve(t *testing.T) {
+	t.Parallel()
+
+	c := cache.NewRepoRootCache("repo")
+
+	// Round-trip the lock twice to confirm BeginResolve/EndResolve pair up
+	// (a missing Unlock would deadlock the second BeginResolve). The lock's
+	// mutual-exclusion semantics are the stdlib's responsibility, not this
+	// test's.
+	c.BeginResolve()
+	c.EndResolve()
+
+	c.BeginResolve()
+	c.EndResolve()
+}
+
+func TestContextRepoRootCache(t *testing.T) {
+	t.Parallel()
+
+	ctx := t.Context()
+
+	// Missing key returns a fresh detached instance, never nil.
+	c := cache.ContextRepoRootCache(ctx, "missing")
+	require.NotNil(t, c)
+	assert.Equal(t, 0, c.Len())
+
+	installed := cache.NewRepoRootCache("installed")
+	ctxWith := context.WithValue(ctx, cache.RepoRootCacheContextKey, installed)
+
+	got := cache.ContextRepoRootCache(ctxWith, cache.RepoRootCacheContextKey)
+	assert.Same(t, installed, got)
 }

internal/git/git.go

@@ -40,14 +40,14 @@ const (
 
 // GitRunner handles git command execution
 type GitRunner struct {
-	goRepo       *git.Repository
-	goStorage    *filesystem.Storage
-	exec         vexec.Exec
-	repoRootOnce *sync.Once
-	repoRootErr  error
-	GitPath      string
-	WorkDir      string
-	repoRoot     string
+	goRepo         *git.Repository
+	goStorage      *filesystem.Storage
+	exec           vexec.Exec
+	repoRootMu     *sync.Mutex
+	GitPath        string
+	WorkDir        string
+	repoRoot       string
+	repoRootCached bool
 }
 
 // NewGitRunner creates a new GitRunner instance. The provided vexec.Exec is
@@ -63,24 +63,24 @@ func NewGitRunner(e vexec.Exec) (*GitRunner, error) {
 	}
 
 	return &GitRunner{
-		GitPath:      gitPath,
-		exec:         e,
-		repoRootOnce: &sync.Once{},
+		GitPath:    gitPath,
+		exec:       e,
+		repoRootMu: &sync.Mutex{},
 	}, nil
 }
 
 // WithWorkDir returns a new GitRunner with the specified working directory
 func (g *GitRunner) WithWorkDir(workDir string) *GitRunner {
 	if g == nil {
-		return &GitRunner{WorkDir: workDir, exec: vexec.NewOSExec(), repoRootOnce: &sync.Once{}}
+		return &GitRunner{WorkDir: workDir, exec: vexec.NewOSExec(), repoRootMu: &sync.Mutex{}}
 	}
 
 	newRunner := *g
 	newRunner.WorkDir = workDir
 	// A different WorkDir may resolve to a different root, so reset the memo.
-	newRunner.repoRootOnce = &sync.Once{}
+	newRunner.repoRootMu = &sync.Mutex{}
 	newRunner.repoRoot = ""
-	newRunner.repoRootErr = nil
+	newRunner.repoRootCached = false
 
 	return &newRunner
 }
@@ -111,19 +111,31 @@ func (g *GitRunner) RequiresGoRepo() error {
 	return nil
 }
 
-// GetRepoRoot returns the root directory of the git repository, memoized
-// per-runner. WithWorkDir clears the memo so a derived runner resolves its
-// own root.
+// GetRepoRoot returns the root directory of the git repository. The
+// successful result is memoized per-runner so subsequent calls skip the
+// `git rev-parse` fork; failures are not cached so callers can retry.
+// WithWorkDir clears the memo so a derived runner resolves its own root.
 func (g *GitRunner) GetRepoRoot(ctx context.Context) (string, error) {
 	if err := g.RequiresWorkDir(); err != nil {
 		return "", err
 	}
 
-	g.repoRootOnce.Do(func() {
-		g.repoRoot, g.repoRootErr = g.runRepoRoot(ctx)
-	})
+	g.repoRootMu.Lock()
+	defer g.repoRootMu.Unlock()
+
+	if g.repoRootCached {
+		return g.repoRoot, nil
+	}
+
+	root, err := g.runRepoRoot(ctx)
+	if err != nil {
+		return "", err
+	}
+
+	g.repoRoot = root
+	g.repoRootCached = true
 
-	return g.repoRoot, g.repoRootErr
+	return root, nil
 }
 
 // runRepoRoot performs the uncached `git rev-parse --show-toplevel`. Use

internal/shell/git.go

@@ -128,10 +128,15 @@ func hasNestedGit(path, root string) (bool, error) {
 			return false, nil
 		}
 
-		if _, err := os.Stat(filepath.Join(current, ".git")); err == nil {
+		_, err := os.Stat(filepath.Join(current, ".git"))
+		if err == nil {
 			return true, nil
 		}
 
+		if !os.IsNotExist(err) {
+			return false, err
+		}
+
 		parent := filepath.Dir(current)
 		if parent == current {
 			return false, nil