Address Copilot review feedback on per-path locking

Normalize chart path keys for the mutex, hold the lock across full
Build/Update operations, and add direct tests for MkdirTemp uniqueness
and equivalent path locking.

Signed-off-by: Anis Khan <2815766+aniskhan001@users.noreply.github.com>

pkg/downloader/manager.go

@@ -89,6 +89,10 @@ type Manager struct {
 //
 // If SkipUpdate is set, this will not update the repository.
 func (m *Manager) Build() error {
+	return m.withChartPathLock(m.build)
+}
+
+func (m *Manager) build() error {
 	c, err := m.loadChartDir()
 	if err != nil {
 		return err
@@ -98,7 +102,7 @@ func (m *Manager) Build() error {
 	// an update.
 	lock := c.Lock
 	if lock == nil {
-		return m.Update()
+		return m.update()
 	}
 
 	// Check that all of the repos we're dependent on actually exist.
@@ -146,7 +150,7 @@ func (m *Manager) Build() error {
 	}
 
 	// Now we need to fetch every package here into charts/
-	return m.downloadAll(lock.Dependencies)
+	return m.downloadAllUnlocked(lock.Dependencies)
 }
 
 // Update updates a local charts directory.
@@ -155,6 +159,10 @@ func (m *Manager) Build() error {
 // negotiate versions based on that. It will download the versions
 // from remote chart repositories unless SkipUpdate is true.
 func (m *Manager) Update() error {
+	return m.withChartPathLock(m.update)
+}
+
+func (m *Manager) update() error {
 	c, err := m.loadChartDir()
 	if err != nil {
 		return err
@@ -202,7 +210,7 @@ func (m *Manager) Update() error {
 	}
 
 	// Now we need to fetch every package here into charts/
-	if err := m.downloadAll(lock.Dependencies); err != nil {
+	if err := m.downloadAllUnlocked(lock.Dependencies); err != nil {
 		return err
 	}
 
@@ -240,26 +248,52 @@ func (m *Manager) resolve(req []*chart.Dependency, repoNames map[string]string)
 	return res.Resolve(req, repoNames)
 }
 
-// chartPathLocks serializes downloadAll calls per ChartPath, since concurrent
-// calls targeting the same path can race on the shared "charts/" directory.
+// chartPathLocks serializes Manager operations per chart directory. Concurrent
+// Build/Update/downloadAll calls targeting the same path can race on the
+// shared "charts/" directory and on the lockfile.
 var chartPathLocks sync.Map // map[string]*sync.Mutex
 
+func normalizeChartPath(chartPath string) string {
+	abs, err := filepath.Abs(chartPath)
+	if err != nil {
+		return filepath.Clean(chartPath)
+	}
+	resolved, err := filepath.EvalSymlinks(abs)
+	if err != nil {
+		return abs
+	}
+	return resolved
+}
+
 func lockForChartPath(chartPath string) *sync.Mutex {
-	lock, _ := chartPathLocks.LoadOrStore(chartPath, &sync.Mutex{})
+	key := normalizeChartPath(chartPath)
+	lock, _ := chartPathLocks.LoadOrStore(key, &sync.Mutex{})
 	return lock.(*sync.Mutex)
 }
 
+func (m *Manager) withChartPathLock(fn func() error) error {
+	lock := lockForChartPath(m.ChartPath)
+	lock.Lock()
+	defer lock.Unlock()
+	return fn()
+}
+
+// tmpChartsDir allocates a unique scratch directory for a downloadAll call.
+func tmpChartsDir(chartPath string) (string, error) {
+	return os.MkdirTemp(chartPath, fmt.Sprintf("tmpcharts-%d-*", os.Getpid()))
+}
+
 // downloadAll takes a list of dependencies and downloads them into charts/
 //
 // It will delete versions of the chart that exist on disk and might cause
 // a conflict.
 func (m *Manager) downloadAll(deps []*chart.Dependency) error {
-	// safeMoveDeps below isn't safe against other calls targeting the same
-	// ChartPath, so serialize per-path here.
-	lock := lockForChartPath(m.ChartPath)
-	lock.Lock()
-	defer lock.Unlock()
+	return m.withChartPathLock(func() error {
+		return m.downloadAllUnlocked(deps)
+	})
+}
 
+func (m *Manager) downloadAllUnlocked(deps []*chart.Dependency) error {
 	repos, err := m.loadChartRepositories()
 	if err != nil {
 		return err
@@ -285,7 +319,7 @@ func (m *Manager) downloadAll(deps []*chart.Dependency) error {
 	// process (e.g. a caller rendering several profiles against one chart
 	// path in parallel), in which case a PID-only suffix is not unique and
 	// callers race on the same directory.
-	tmpPath, err := os.MkdirTemp(m.ChartPath, fmt.Sprintf("tmpcharts-%d-*", os.Getpid()))
+	tmpPath, err := tmpChartsDir(m.ChartPath)
 	if err != nil {
 		return fmt.Errorf("unable to create temporary directory in '%s': %w", m.ChartPath, err)
 	}

pkg/downloader/manager_test.go

@@ -235,16 +235,10 @@ version: 0.1.0`
 	require.Error(t, m.downloadAll([]*chart.Dependency{badLocalDep}), "Expected error for bad dependency name")
 }
 
-// TestDownloadAllConcurrent reproduces a race that isn't covered by the
-// os.Getpid()-suffixed tmpcharts fix for #13110: multiple goroutines calling
-// downloadAll for the *same* chart path from within a single process (e.g. a
-// caller like Skaffold rendering several profiles against one chart in
-// parallel) all shared a PID, so they used to race on the same
-// "tmpcharts-<pid>" directory. One goroutine's `defer os.RemoveAll(tmpPath)`
-// could delete the directory out from under another goroutine's in-flight
-// download, surfacing as "lstat .../tmpcharts-<pid>: no such file or
-// directory". Each call must get its own unique tmp directory regardless of
-// PID.
+// TestDownloadAllConcurrent verifies concurrent downloadAll calls for the same
+// chart path complete without error and leave no leftover tmpcharts-* dirs.
+// Per-path serialization is provided by withChartPathLock; see
+// TestTmpChartsDirConcurrent for the MkdirTemp uniqueness guarantee.
 func TestDownloadAllConcurrent(t *testing.T) {
 	chartPath := t.TempDir()
 
@@ -389,6 +383,46 @@ func TestDownloadAllConcurrentVersionChurn(t *testing.T) {
 	assert.Len(t, found, 1, "expected exactly one version of %s in charts/, found %v", depName, found)
 }
 
+func TestTmpChartsDirConcurrent(t *testing.T) {
+	chartPath := t.TempDir()
+
+	const concurrency = 32
+	paths := make([]string, concurrency)
+	var wg sync.WaitGroup
+	for i := 0; i < concurrency; i++ {
+		wg.Add(1)
+		go func(i int) {
+			defer wg.Done()
+			path, err := tmpChartsDir(chartPath)
+			require.NoError(t, err)
+			paths[i] = path
+			require.NoError(t, os.RemoveAll(path))
+		}(i)
+	}
+	wg.Wait()
+
+	seen := make(map[string]struct{}, concurrency)
+	for _, path := range paths {
+		require.NotEmpty(t, path)
+		_, dup := seen[path]
+		require.False(t, dup, "duplicate tmp dir: %s", path)
+		seen[path] = struct{}{}
+	}
+}
+
+func TestLockForChartPathEquivalentPaths(t *testing.T) {
+	chartPath := t.TempDir()
+	linkPath := filepath.Join(filepath.Dir(chartPath), "chart-link")
+	require.NoError(t, os.Symlink(chartPath, linkPath))
+
+	lockA := lockForChartPath(chartPath)
+	lockB := lockForChartPath(linkPath)
+	lockC := lockForChartPath(filepath.Join(chartPath, "..", filepath.Base(chartPath)))
+
+	require.Same(t, lockA, lockB)
+	require.Same(t, lockA, lockC)
+}
+
 func TestUpdateBeforeBuild(t *testing.T) {
 	// Set up a fake repo
 	srv := repotest.NewTempServer(