fix(downloader): make tmpcharts dir unique per call, not just per process

Manager.downloadAll derives its scratch directory as tmpcharts-<pid>
(added in #13120 to fix #13110, a race between separate helm process
invocations sharing a chart directory). That fix does not cover the
case where downloadAll is called concurrently by multiple goroutines
within a single process - e.g. a caller embedding Helm as a library
that renders several profiles against the same chart path in parallel
(this is how Skaffold's built-in Helm renderer works). All such calls
share one PID, so they still resolve to the same tmpcharts-<pid> path
and race on it: one goroutine's deferred os.RemoveAll can delete the
directory out from under another goroutine's in-flight download,
surfacing as:

  lstat .../tmpcharts-<pid>: no such file or directory

Switch to os.MkdirTemp, which atomically allocates a directory with a
guaranteed-unique name, while keeping the PID as a prefix for
debuggability. Adds TestDownloadAllConcurrent, which reproduces the
race under -race with the old code and passes reliably with the fix.

Fixes #32532

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

pkg/cmd/dependency_update_test.go

@@ -17,7 +17,6 @@ package cmd
 
 import (
 	"fmt"
-	"io/fs"
 	"os"
 	"path/filepath"
 	"testing"
@@ -176,10 +175,14 @@ func TestDependencyUpdateCmd_DoNotDeleteOldChartsOnError(t *testing.T) {
 		require.Equal(t, file.Name(), dependencies[index], "Chart dependency %s not matching %s", dependencies[index], file.Name())
 	}
 
-	// Make sure tmpcharts-x is deleted
-	tmpPath := filepath.Join(dir(chartname), fmt.Sprintf("tmpcharts-%d", os.Getpid()))
-	_, err = os.Stat(tmpPath)
-	require.ErrorIs(t, err, fs.ErrNotExist, "tmpcharts dir still exists")
+	// Make sure no tmpcharts-* dir is left behind. The suffix is now a random
+	// MkdirTemp-generated string (not just the PID), since downloadAll can be
+	// called concurrently by multiple goroutines within the same process.
+	entries, err := os.ReadDir(dir(chartname))
+	require.NoError(t, err)
+	for _, entry := range entries {
+		assert.NotContains(t, entry.Name(), fmt.Sprintf("tmpcharts-%d", os.Getpid()), "tmpcharts dir still exists: %s", entry.Name())
+	}
 }
 
 func TestDependencyUpdateCmd_WithRepoThatWasNotAdded(t *testing.T) {

pkg/downloader/manager.go

@@ -251,7 +251,6 @@ func (m *Manager) downloadAll(deps []*chart.Dependency) error {
 	}
 
 	destPath := filepath.Join(m.ChartPath, "charts")
-	tmpPath := filepath.Join(m.ChartPath, fmt.Sprintf("tmpcharts-%d", os.Getpid()))
 
 	// Check if 'charts' directory is not actually a directory. If it does not exist, create it.
 	if fi, err := os.Stat(destPath); err == nil {
@@ -266,8 +265,13 @@ func (m *Manager) downloadAll(deps []*chart.Dependency) error {
 		return fmt.Errorf("unable to retrieve file info for '%s': %w", destPath, err)
 	}
 
-	// Prepare tmpPath
-	if err := os.MkdirAll(tmpPath, 0o755); err != nil {
+	// Prepare tmpPath. Use MkdirTemp rather than a PID-derived name: multiple
+	// downloadAll calls can run concurrently as goroutines within the same
+	// 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()))
+	if err != nil {
 		return err
 	}
 	defer os.RemoveAll(tmpPath)

pkg/downloader/manager_test.go

@@ -20,6 +20,8 @@ import (
 	"io/fs"
 	"os"
 	"path/filepath"
+	"strings"
+	"sync"
 	"testing"
 	"time"
 
@@ -232,6 +234,79 @@ 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.
+func TestDownloadAllConcurrent(t *testing.T) {
+	chartPath := t.TempDir()
+
+	signtest, err := loader.LoadDir(filepath.Join("testdata", "signtest"))
+	require.NoError(t, err)
+	require.NoError(t, chartutil.SaveDir(signtest, filepath.Join(chartPath, "testdata")))
+
+	local, err := loader.LoadDir(filepath.Join("testdata", "local-subchart"))
+	require.NoError(t, err)
+	require.NoError(t, chartutil.SaveDir(local, filepath.Join(chartPath, "charts")))
+
+	// Each goroutine gets its own *chart.Dependency instances: downloadAll
+	// mutates dep.Version in place, so sharing pointers across concurrent
+	// calls would race on that unrelated field and mask the tmpPath race
+	// this test targets.
+	newDeps := func() []*chart.Dependency {
+		return []*chart.Dependency{
+			{
+				Name:       signtest.Name(),
+				Repository: "file://./testdata/signtest",
+				Version:    signtest.Metadata.Version,
+			},
+			{
+				Name:       local.Name(),
+				Repository: "",
+				Version:    local.Metadata.Version,
+			},
+		}
+	}
+
+	const concurrency = 8
+	errs := make([]error, concurrency)
+	var wg sync.WaitGroup
+	for i := 0; i < concurrency; i++ {
+		wg.Add(1)
+		go func(i int) {
+			defer wg.Done()
+			m := &Manager{
+				Out:              new(bytes.Buffer),
+				RepositoryConfig: repoConfig,
+				RepositoryCache:  repoCache,
+				ChartPath:        chartPath,
+			}
+			errs[i] = m.downloadAll(newDeps())
+		}(i)
+	}
+	wg.Wait()
+
+	for i, err := range errs {
+		assert.NoError(t, err, "concurrent downloadAll call %d failed", i)
+	}
+
+	_, err = os.Stat(filepath.Join(chartPath, "charts", "signtest-0.1.0.tgz"))
+	require.NotErrorIs(t, err, fs.ErrNotExist)
+
+	// No leftover tmpcharts-* directories from any of the concurrent calls.
+	entries, err := os.ReadDir(chartPath)
+	require.NoError(t, err)
+	for _, entry := range entries {
+		assert.False(t, strings.HasPrefix(entry.Name(), "tmpcharts-"), "leftover tmp dir: %s", entry.Name())
+	}
+}
+
 func TestUpdateBeforeBuild(t *testing.T) {
 	// Set up a fake repo
 	srv := repotest.NewTempServer(