fix: clean up chartify temp directories after helm operations (#2668)

fix: clean up chartify temp directories after helm operations (#1799) Chartify creates temporary output directories (e.g. /tmp/chartify<random>/) during chart preparation for commands like build, template, and diff. These directories were never tracked for cleanup, causing disk space to accumulate over time with thousands of orphaned chartify* folders. The existing clean() closure in PrepareChartify only removed generated values files, not the chartify output directory itself. The chartified chart must survive until all helm operations complete, so it could not be removed during chart preparation. This change: - Adds chartifyTempDirTracker to HelmState (pointer-based to avoid copy-lock issues from HelmState being copied in several places) - Tracks chartify output dirs via addChartifyTempDir() after c.Chartify() succeeds in processChartification - Cleans them up via CleanupChartifyTempDirs() in WithPreparedCharts after all helm operations complete - Also removes empty parent temp directories (e.g. /tmp/chartify<random>/) Fixes #1799 Signed-off-by: yxxhero <aiopsclub@163.com>
2026-09-30 07:08:38 +02:00 · 2026-06-28 11:55:31 +08:00
parent 76613bd0bb
commit 0cf330611e
3 changed files with 243 additions and 0 deletions
@@ -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
@@ -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<rand>/<id>
+	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<rand>/)
+	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)
+	}
+}
@@ -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<random>) 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{