Merge df4be0b5e7 into 62c7e845d1

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

@@ -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
@@ -203,7 +211,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
 	}
 
@@ -241,18 +249,58 @@ func (m *Manager) resolve(req []*chart.Dependency, repoNames map[string]string)
 	return res.Resolve(req, repoNames)
 }
 
+// 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 {
+	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 {
+	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
 	}
 
 	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 {
@@ -267,9 +315,14 @@ 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 {
-		return err
+	// 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 := tmpChartsDir(m.ChartPath)
+	if err != nil {
+		return fmt.Errorf("unable to create temporary directory in '%s': %w", m.ChartPath, err)
 	}
 	defer os.RemoveAll(tmpPath)
 

pkg/downloader/manager_test.go

@@ -17,9 +17,12 @@ package downloader
 
 import (
 	"bytes"
+	"fmt"
 	"io/fs"
 	"os"
 	"path/filepath"
+	"strings"
+	"sync"
 	"testing"
 	"time"
 
@@ -232,6 +235,194 @@ version: 0.1.0`
 	require.Error(t, m.downloadAll([]*chart.Dependency{badLocalDep}), "Expected error for bad dependency name")
 }
 
+// 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()
+
+	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.NoError(t, err)
+
+	// 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())
+	}
+}
+
+// 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 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(