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 2e94d26ae..a27de3d87 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 @@ -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) diff --git a/pkg/downloader/manager_test.go b/pkg/downloader/manager_test.go index e40bbbac1..e242ed251 100644 --- a/pkg/downloader/manager_test.go +++ b/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(