fix: remove partially extracted chart when helm pull fails (#2805)

extracts in place, so a failed fetch can leave a partial chart in the cache. `forcedDownloadChart` returns the error but never removes the partial directory, and `findChartDirectory` accepts any directory containing a Chart.yaml, so the next run adopts the leftover as a valid cache entry and renders zero manifests without error. Remove the partial download while still holding the exclusive chart lock so the failure stays a failure.

Co-authored-by: yxxhero <aiopsclub@163.com>
Signed-off-by: Johannes Schmidt <jschmidt@canarytechnologies.com>
Signed-off-by: yxxhero <aiopsclub@163.com>

pkg/state/partial_chart_download_test.go

@@ -0,0 +1,80 @@
+package state
+
+import (
+	"errors"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	"go.uber.org/zap"
+
+	"github.com/helmfile/helmfile/pkg/exectest"
+	"github.com/helmfile/helmfile/pkg/filesystem"
+)
+
+// partialFetchHelm simulates `helm pull --untar` dying part-way through the
+// extraction: Chart.yaml (the first entry in a chart archive) lands on disk,
+// nothing after it does, and the fetch reports an error.
+type partialFetchHelm struct {
+	*exectest.Helm
+}
+
+func (h *partialFetchHelm) Fetch(chart string, flags ...string) error {
+	var untarDir string
+	for i, f := range flags {
+		if f == "--untardir" && i+1 < len(flags) {
+			untarDir = flags[i+1]
+			break
+		}
+	}
+	if untarDir != "" {
+		chartDir := filepath.Join(untarDir, "mychart")
+		if err := os.MkdirAll(chartDir, 0755); err != nil {
+			return err
+		}
+		partial := "apiVersion: v2\nname: mychart\nversion: 1.0.0\n"
+		if err := os.WriteFile(filepath.Join(chartDir, "Chart.yaml"), []byte(partial), 0644); err != nil {
+			return err
+		}
+	}
+	return errors.New("fetch failed")
+}
+
+// A failed fetch must not leave a partially extracted chart in the cache.
+//
+// Chart.yaml is the first entry in a chart archive, so an interrupted extraction
+// typically leaves a complete Chart.yaml and nothing else. findChartDirectory
+// accepts any directory containing a Chart.yaml, so such leftovers are adopted as
+// a valid cache entry by the next run -- which then renders zero manifests and
+// exits 0. Under --skip-refresh, or anywhere below the shared cache dir where
+// isSharedCachePath suppresses refresh, that state is permanent.
+func TestForcedDownloadChart_RemovesPartialDownloadOnFetchError(t *testing.T) {
+	cacheRoot := t.TempDir()
+
+	st := &HelmState{
+		fs:       filesystem.DefaultFileSystem(),
+		logger:   zap.NewNop().Sugar(),
+		Releases: []ReleaseSpec{{Name: "app", Chart: "myrepo/mychart"}},
+	}
+
+	release := &st.Releases[0]
+	helm := &partialFetchHelm{Helm: &exectest.Helm{}}
+
+	_, err := st.forcedDownloadChart("myrepo/mychart", cacheRoot, release, helm, ChartPrepareOptions{
+		SkipRefresh: true,
+	})
+	require.Error(t, err, "a failed fetch must surface as an error")
+
+	entries, readErr := os.ReadDir(cacheRoot)
+	require.NoError(t, readErr)
+	for _, e := range entries {
+		if !e.IsDir() {
+			continue
+		}
+		leftover := filepath.Join(cacheRoot, e.Name())
+		_, findErr := findChartDirectory(leftover)
+		require.Error(t, findErr,
+			"partial download at %s was left behind and would be reused as a valid chart", leftover)
+	}
+}

pkg/state/state.go

@@ -2114,6 +2114,14 @@ func (st *HelmState) forcedDownloadChart(chartName, dir string, release *Release
 	fetchFlags := st.chartFetchFlags(release)
 	fetchFlags = append(fetchFlags, "--untar", "--untardir", chartPath)
 	if err := helm.Fetch(chartName, fetchFlags...); err != nil {
+		// `helm pull --untar` extracts in place, so a failed fetch can leave a partially
+		// extracted chart behind. Chart.yaml is the first entry in a chart archive, so the
+		// leftover often contains a complete Chart.yaml and nothing else -- which
+		// findChartDirectory accepts, making the next run reuse it as a valid cache entry
+		// and render zero manifests without error. Remove it so the failure stays a failure.
+		if rmErr := os.RemoveAll(chartPath); rmErr != nil {
+			st.logger.Warnf("failed to remove partial chart download at %s: %v", chartPath, rmErr)
+		}
 		lockResult.Release(st.logger)
 		return "", err
 	}