From 7b067935b382624fe5689070c750cf83bfa483b1 Mon Sep 17 00:00:00 2001 From: Anis Khan <2815766+aniskhan001@users.noreply.github.com> Date: Wed, 12 Aug 2026 15:56:50 +0200 Subject: [PATCH] fix(downloader): make tmpcharts dir unique per call, not just per process Manager.downloadAll derives its scratch directory as tmpcharts- (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- 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-: 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 | 13 +++--- pkg/downloader/manager.go | 10 +++-- pkg/downloader/manager_test.go | 75 +++++++++++++++++++++++++++++++ 3 files changed, 90 insertions(+), 8 deletions(-) diff --git a/pkg/cmd/dependency_update_test.go b/pkg/cmd/dependency_update_test.go index 34575ad4d..7610159df 100644 --- a/pkg/cmd/dependency_update_test.go +++ b/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) { diff --git a/pkg/downloader/manager.go b/pkg/downloader/manager.go index a1fa0ddbd..46c4d4663 100644 --- a/pkg/downloader/manager.go +++ b/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) diff --git a/pkg/downloader/manager_test.go b/pkg/downloader/manager_test.go index e40bbbac1..163958f47 100644 --- a/pkg/downloader/manager_test.go +++ b/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-" 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-: 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(