diff --git a/pkg/app/run.go b/pkg/app/run.go index 1b120672..590c10bc 100644 --- a/pkg/app/run.go +++ b/pkg/app/run.go @@ -95,6 +95,12 @@ func (r *Run) WithPreparedCharts(helmfileCommand string, opts state.ChartPrepare return err } + // Ensure chartify temp directories are always cleaned up, even when chart + // preparation or helm operations fail. The deferred call runs at function + // exit — after f() and TriggerGlobalCleanupEvent — so chartified charts + // remain available for the entire operation lifecycle. See issue #1799. + defer r.state.CleanupChartifyTempDirs() + releaseToChart, err := r.prepareChartsIfNeeded(helmfileCommand, dir, opts.Concurrency, opts) if err != nil { return err diff --git a/pkg/state/issue_1799_test.go b/pkg/state/issue_1799_test.go new file mode 100644 index 00000000..d1cfd097 --- /dev/null +++ b/pkg/state/issue_1799_test.go @@ -0,0 +1,154 @@ +package state + +import ( + "os" + "path/filepath" + "sync" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/zap" + + "github.com/helmfile/helmfile/pkg/filesystem" +) + +// TestChartifyTempDirCleanup verifies that chartify output directories tracked +// via addChartifyTempDir are removed by CleanupChartifyTempDirs, including the +// parent temp directory when it becomes empty. Regression test for issue #1799. +func TestChartifyTempDirCleanup(t *testing.T) { + st := &HelmState{ + logger: zap.NewNop().Sugar(), + fs: filesystem.DefaultFileSystem(), + } + + // Simulate a chartify output directory: /tmp/chartify/ + parent := t.TempDir() + dir := filepath.Join(parent, "release-name-12345") + require.NoError(t, os.MkdirAll(dir, 0755)) + // Add some content so RemoveAll has something to remove + require.NoError(t, os.WriteFile(filepath.Join(dir, "Chart.yaml"), []byte("apiVersion: v2\n"), 0644)) + + st.addChartifyTempDir(dir) + + // Before cleanup, the directory exists + _, err := os.Stat(dir) + assert.NoError(t, err) + + st.CleanupChartifyTempDirs() + + // After cleanup, the chartify output dir is gone + _, err = os.Stat(dir) + assert.True(t, os.IsNotExist(err), "chartify output dir should be removed") + + // The parent temp dir should also be removed since it is now empty + _, err = os.Stat(parent) + assert.True(t, os.IsNotExist(err), "empty parent temp dir should be removed") +} + +// TestChartifyTempDirCleanupMultipleReleases verifies that cleanup handles +// multiple chartify output directories correctly, including shared parents. +func TestChartifyTempDirCleanupMultipleReleases(t *testing.T) { + st := &HelmState{ + logger: zap.NewNop().Sugar(), + fs: filesystem.DefaultFileSystem(), + } + + // Two chartify output dirs under the SAME parent (simulating concurrent + // releases that share /tmp/chartify/) + parent := t.TempDir() + dir1 := filepath.Join(parent, "release-a-111") + dir2 := filepath.Join(parent, "release-b-222") + require.NoError(t, os.MkdirAll(dir1, 0755)) + require.NoError(t, os.MkdirAll(dir2, 0755)) + require.NoError(t, os.WriteFile(filepath.Join(dir1, "Chart.yaml"), []byte("apiVersion: v2\n"), 0644)) + require.NoError(t, os.WriteFile(filepath.Join(dir2, "Chart.yaml"), []byte("apiVersion: v2\n"), 0644)) + + st.addChartifyTempDir(dir1) + st.addChartifyTempDir(dir2) + + st.CleanupChartifyTempDirs() + + for _, d := range []string{dir1, dir2} { + _, err := os.Stat(d) + assert.True(t, os.IsNotExist(err), "chartify output dir %s should be removed", d) + } + // Parent should also be cleaned since both children are gone + _, err := os.Stat(parent) + assert.True(t, os.IsNotExist(err), "parent should be removed when all children removed") +} + +// TestChartifyTempDirCleanupNoop verifies CleanupChartifyTempDirs is safe to call +// when no directories were tracked (e.g. no chartify runs occurred). +func TestChartifyTempDirCleanupNoop(t *testing.T) { + st := &HelmState{ + logger: zap.NewNop().Sugar(), + fs: filesystem.DefaultFileSystem(), + } + + // Should not panic + assert.NotPanics(t, func() { + st.CleanupChartifyTempDirs() + }) +} + +// TestChartifyTempDirCleanupIdempotent verifies that calling cleanup twice is safe. +func TestChartifyTempDirCleanupIdempotent(t *testing.T) { + st := &HelmState{ + logger: zap.NewNop().Sugar(), + fs: filesystem.DefaultFileSystem(), + } + + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "Chart.yaml"), []byte("apiVersion: v2\n"), 0644)) + + st.addChartifyTempDir(dir) + st.CleanupChartifyTempDirs() + // Second call should be a no-op, not an error + assert.NotPanics(t, func() { + st.CleanupChartifyTempDirs() + }) +} + +// TestChartifyTempDirConcurrentTracking verifies that concurrent calls to +// addChartifyTempDir (as happens with parallel chart-preparation workers) do +// not lose any tracked directories. The tracker must be pre-initialized before +// concurrent access — see PrepareCharts. Regression test for issue #1799. +func TestChartifyTempDirConcurrentTracking(t *testing.T) { + st := &HelmState{ + logger: zap.NewNop().Sugar(), + fs: filesystem.DefaultFileSystem(), + } + + // Pre-initialize the tracker as PrepareCharts does before launching workers. + st.chartifyTempDirs = &chartifyTempDirTracker{} + + const n = 50 + dirs := make([]string, n) + for i := range dirs { + dirs[i] = t.TempDir() + } + + var wg sync.WaitGroup + for i := 0; i < n; i++ { + wg.Add(1) + go func(dir string) { + defer wg.Done() + st.addChartifyTempDir(dir) + }(dirs[i]) + } + wg.Wait() + + // All n directories must be tracked — none lost to a race. + st.chartifyTempDirs.mu.Lock() + got := len(st.chartifyTempDirs.dirs) + st.chartifyTempDirs.mu.Unlock() + assert.Equal(t, n, got, "all concurrently tracked dirs must be present") + + // Cleanup should remove all of them. + st.CleanupChartifyTempDirs() + for i, d := range dirs { + _, err := os.Stat(d) + assert.True(t, os.IsNotExist(err), "dir %d (%s) should have been removed", i, d) + } +} diff --git a/pkg/state/state.go b/pkg/state/state.go index 4aba56bd..ecff60d2 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -154,12 +154,28 @@ type HelmState struct { kubeconfig string + // chartifyTempDirs tracks temporary directories created by chartify during + // chart preparation. These directories contain the chartified charts and must + // survive until all helm operations complete, after which they are cleaned up + // via CleanupChartifyTempDirs. It is a pointer so that copying HelmState + // (which happens in several places) does not copy the embedded mutex. + // See issue #1799. + chartifyTempDirs *chartifyTempDirTracker + // RenderedValues is the helmfile-wide values that is `.Values` // which is accessible from within the whole helmfile go template. // Note that this is usually computed by DesiredStateLoader from ReleaseSetSpec.Env RenderedValues map[string]any } +// chartifyTempDirTracker holds the set of chartify output directories to be +// cleaned up after helm operations complete. The mutex guards concurrent access +// from chart-preparation workers. See issue #1799. +type chartifyTempDirTracker struct { + mu sync.Mutex + dirs []string +} + func (st *HelmState) SetKubeconfig(kubeconfig string) { st.kubeconfig = kubeconfig } @@ -1931,6 +1947,10 @@ func (st *HelmState) processChartification(chartification *Chartify, release *Re } chartPath = out + // Track the chartify output directory for cleanup after all helm operations + // complete. The chartified chart at `out` is used by subsequent helm commands, + // so it cannot be removed yet. See issue #1799. + st.addChartifyTempDir(out) // Skip `helm dep build` and `helm dep up` altogether when the chart is from remote or the dep is // explicitly skipped. buildDeps := !skipDeps @@ -2193,6 +2213,10 @@ func (st *HelmState) PrepareCharts(helm helmexec.Interface, dir string, concurre releases = filterReleasesForBuild(releases) } + // Initialize the chartify temp dir tracker before concurrent workers start, + // so that all workers share a single tracker instance. See issue #1799. + st.chartifyTempDirs = &chartifyTempDirTracker{} + var prepareChartInfoMutex sync.Mutex prepareChartInfo := make(map[PrepareChartKey]string, len(releases)) @@ -4529,6 +4553,65 @@ func (st *HelmState) removeFiles(files []string) { } } +// addChartifyTempDir records a chartify output directory for deferred cleanup. +// The directory is removed by CleanupChartifyTempDirs after all helm operations +// have completed, since the chartified chart may still be in use. See issue #1799. +func (st *HelmState) addChartifyTempDir(dir string) { + if st.chartifyTempDirs == nil { + st.chartifyTempDirs = &chartifyTempDirTracker{} + } + st.chartifyTempDirs.mu.Lock() + defer st.chartifyTempDirs.mu.Unlock() + st.chartifyTempDirs.dirs = append(st.chartifyTempDirs.dirs, dir) +} + +// CleanupChartifyTempDirs removes all chartify output directories tracked via +// addChartifyTempDir. It should be called after all helm operations complete. +// See issue #1799. +func (st *HelmState) CleanupChartifyTempDirs() { + if st.chartifyTempDirs == nil { + return + } + + st.chartifyTempDirs.mu.Lock() + dirs := st.chartifyTempDirs.dirs + st.chartifyTempDirs.dirs = nil + st.chartifyTempDirs.mu.Unlock() + + seen := make(map[string]bool, len(dirs)) + parents := make([]string, 0, len(dirs)) + + for _, dir := range dirs { + if dir == "" || seen[dir] { + continue + } + seen[dir] = true + + if err := st.fs.RemoveAll(dir); err != nil { + st.logger.Warnf("Removing chartify temp dir %s: %v", dir, err) + } else { + st.logger.Debugf("Removed chartify temp dir %s", dir) + } + + // Collect the parent temp directory (e.g. /tmp/chartify) for + // later removal. os.Remove only succeeds on empty directories, so we + // attempt it after all child directories have been removed. + parent := filepath.Dir(dir) + if parent != "" && parent != "/" && !seen[parent] { + seen[parent] = true + parents = append(parents, parent) + } + } + + for _, parent := range parents { + if err := os.Remove(parent); err != nil { + st.logger.Debugf("Not removing parent temp dir %s: %v", parent, err) + } else { + st.logger.Debugf("Removed parent temp dir %s", parent) + } + } +} + func (c *MissingFileHandlerConfig) resolveFileOptions() []resolveFileOption { if c == nil { return []resolveFileOption{