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 1/6] 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( From a7e8107b5a5447ffa161fe0c5b0e868a047c653b Mon Sep 17 00:00:00 2001 From: Anis Khan <2815766+aniskhan001@users.noreply.github.com> Date: Wed, 12 Aug 2026 17:37:41 +0200 Subject: [PATCH 2/6] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Anis Khan <2815766+aniskhan001@users.noreply.github.com> --- pkg/downloader/manager_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/downloader/manager_test.go b/pkg/downloader/manager_test.go index 163958f47..2d698aa68 100644 --- a/pkg/downloader/manager_test.go +++ b/pkg/downloader/manager_test.go @@ -297,7 +297,7 @@ func TestDownloadAllConcurrent(t *testing.T) { } _, err = os.Stat(filepath.Join(chartPath, "charts", "signtest-0.1.0.tgz")) - require.NotErrorIs(t, err, fs.ErrNotExist) + require.NoError(t, err) // No leftover tmpcharts-* directories from any of the concurrent calls. entries, err := os.ReadDir(chartPath) From 7a0bf95a5e0b7e2606f47516e71364a5a5bd397d Mon Sep 17 00:00:00 2001 From: Anis Khan <2815766+aniskhan001@users.noreply.github.com> Date: Wed, 12 Aug 2026 19:20:41 +0200 Subject: [PATCH 3/6] Update pkg/downloader/manager.go Co-authored-by: Terry Howe Signed-off-by: Anis Khan <2815766+aniskhan001@users.noreply.github.com> --- pkg/downloader/manager.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/downloader/manager.go b/pkg/downloader/manager.go index 46c4d4663..b6c052c66 100644 --- a/pkg/downloader/manager.go +++ b/pkg/downloader/manager.go @@ -272,7 +272,7 @@ func (m *Manager) downloadAll(deps []*chart.Dependency) error { // callers race on the same directory. tmpPath, err := os.MkdirTemp(m.ChartPath, fmt.Sprintf("tmpcharts-%d-*", os.Getpid())) if err != nil { - return err + return fmt.Errorf("unable to create temporary directory in '%s': %w", m.ChartPath, err) } defer os.RemoveAll(tmpPath) From 280cdc4f0c9de42146ca8727a2e551746c3533fc Mon Sep 17 00:00:00 2001 From: Anis Khan <2815766+aniskhan001@users.noreply.github.com> Date: Wed, 12 Aug 2026 19:45:34 +0200 Subject: [PATCH 4/6] fix(downloader): serialize downloadAll per ChartPath safeMoveDeps reads the destination charts/ directory and then moves in new charts and deletes ones missing from that snapshot. Separate Manager instances calling downloadAll concurrently against the same ChartPath can race on this read-then-write, e.g. one call's freshly moved-in chart being deleted by another call's "delete outdated charts" pass because it wasn't in that call's earlier snapshot. Signed-off-by: Anis Khan <2815766+aniskhan001@users.noreply.github.com> --- pkg/downloader/manager.go | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/pkg/downloader/manager.go b/pkg/downloader/manager.go index b6c052c66..74aa8a8c3 100644 --- a/pkg/downloader/manager.go +++ b/pkg/downloader/manager.go @@ -240,11 +240,26 @@ 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. +var chartPathLocks sync.Map // map[string]*sync.Mutex + +func lockForChartPath(chartPath string) *sync.Mutex { + lock, _ := chartPathLocks.LoadOrStore(chartPath, &sync.Mutex{}) + return lock.(*sync.Mutex) +} + // 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() + repos, err := m.loadChartRepositories() if err != nil { return err From fdb9929a939084de354eea201b0d8e1d526c1ccd Mon Sep 17 00:00:00 2001 From: Anis Khan <2815766+aniskhan001@users.noreply.github.com> Date: Wed, 12 Aug 2026 21:00:05 +0200 Subject: [PATCH 5/6] add concurrent churn test Signed-off-by: Anis Khan <2815766+aniskhan001@users.noreply.github.com> --- pkg/downloader/manager_test.go | 82 ++++++++++++++++++++++++++++++++++ 1 file changed, 82 insertions(+) diff --git a/pkg/downloader/manager_test.go b/pkg/downloader/manager_test.go index 2d698aa68..77d001564 100644 --- a/pkg/downloader/manager_test.go +++ b/pkg/downloader/manager_test.go @@ -17,6 +17,7 @@ package downloader import ( "bytes" + "fmt" "io/fs" "os" "path/filepath" @@ -307,6 +308,87 @@ func TestDownloadAllConcurrent(t *testing.T) { } } +// TestDownloadAllConcurrentVersionChurn guards the race fixed by serializing +// downloadAll per ChartPath. safeMoveDeps snapshots the destination charts/ +// directory, moves in files matching its own deps (by filename, which +// embeds the version), then deletes any dest file from that snapshot that +// isn't one of those filenames - including older versions of a dependency +// it just replaced. +// +// This is racy when concurrent downloadAll calls for the same ChartPath +// resolve the same dependency to different versions between calls (e.g. a +// caller like Skaffold repeatedly rendering a chart while the upstream repo +// index is being updated). Call A moves in dep-2.0.0.tgz; call B's +// destination snapshot predates A's write and still expects dep-1.0.0.tgz, +// so once B commits, its own delete-check treats A's freshly written +// dep-2.0.0.tgz as a stranger and removes it - even though a "dep" chart is +// still one of B's deps, just resolved to a different version. +// +// Each goroutine here resolves the same dependency name to a distinct +// version, so if the race occurs, later versions written by other +// goroutines end up deleted, leaving fewer than `concurrency` chart files +// (or the wrong one) in charts/ afterward. +func TestDownloadAllConcurrentVersionChurn(t *testing.T) { + chartPath := t.TempDir() + + const concurrency = 8 + const depName = "churn-dep" + deps := make([]*chart.Dependency, concurrency) + for i := 0; i < concurrency; i++ { + version := fmt.Sprintf("0.1.%d", i) + src := &chart.Chart{ + Metadata: &chart.Metadata{ + Name: depName, + Version: version, + APIVersion: "v2", + }, + } + srcParent := filepath.Join(chartPath, fmt.Sprintf("src-%d", i)) + require.NoError(t, os.MkdirAll(srcParent, 0o755)) + require.NoError(t, chartutil.SaveDir(src, srcParent)) + + deps[i] = &chart.Dependency{ + Name: depName, + Repository: fmt.Sprintf("file://./src-%d/%s", i, depName), + Version: version, + } + } + + 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([]*chart.Dependency{deps[i]}) + }(i) + } + wg.Wait() + + for i, err := range errs { + assert.NoError(t, err, "concurrent downloadAll call %d failed", i) + } + + // Exactly one version of depName should remain: whichever call's + // safeMoveDeps ran last should have left its own version behind, and no + // call should have raced another's write out of existence. + entries, err := os.ReadDir(filepath.Join(chartPath, "charts")) + require.NoError(t, err) + var found []string + for _, entry := range entries { + if strings.HasPrefix(entry.Name(), depName+"-") { + found = append(found, entry.Name()) + } + } + assert.Len(t, found, 1, "expected exactly one version of %s in charts/, found %v", depName, found) +} + func TestUpdateBeforeBuild(t *testing.T) { // Set up a fake repo srv := repotest.NewTempServer( From df4be0b5e751e2c0aaa1c0fe65639f208113dec6 Mon Sep 17 00:00:00 2001 From: Anis Khan <2815766+aniskhan001@users.noreply.github.com> Date: Fri, 28 Aug 2026 15:44:34 +0200 Subject: [PATCH 6/6] 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 | 58 +++++++++++++++++++++++++++------- pkg/downloader/manager_test.go | 54 +++++++++++++++++++++++++------ 2 files changed, 90 insertions(+), 22 deletions(-) diff --git a/pkg/downloader/manager.go b/pkg/downloader/manager.go index 74aa8a8c3..ae7433747 100644 --- a/pkg/downloader/manager.go +++ b/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) } diff --git a/pkg/downloader/manager_test.go b/pkg/downloader/manager_test.go index 77d001564..e242ed251 100644 --- a/pkg/downloader/manager_test.go +++ b/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-" 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. +// 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(