fix(stack): tolerate transient helm repo update failures, keep cluster running

internal/helmcmd/helmcmd.go

@@ -11,10 +11,13 @@ package helmcmd
 
 import (
 	"fmt"
+	"os"
 	"os/exec"
 	"regexp"
 	"strconv"
 	"strings"
+
+	"gopkg.in/yaml.v3"
 )
 
 // versionRE matches the major number in `helm version --short` output, e.g.
@@ -63,3 +66,122 @@ func SyncFlagsForVersion(helmBinary string) []string {
 	}
 	return []string{"--sync-args=--force-conflicts"}
 }
+
+// helmfileRepo mirrors the shape of each entry under the top-level
+// `repositories:` key in a helmfile.yaml. Only the fields we need are decoded;
+// extra keys (oci, username, passwordRef, ...) are ignored.
+type helmfileRepo struct {
+	Name string `yaml:"name"`
+	URL  string `yaml:"url"`
+}
+
+// helmfileDoc is the minimal shape of helmfile.yaml we care about for the
+// repo-update preflight: just the `repositories:` block.
+type helmfileDoc struct {
+	Repositories []helmfileRepo `yaml:"repositories"`
+}
+
+// ParseHelmfileRepos extracts (name, url) entries from a helmfile.yaml file.
+// Repos without both a name and a URL (e.g. OCI-only refs) are skipped — they
+// are not added via `helm repo add` so they don't participate in the
+// `helm repo update` path that this package guards against.
+func ParseHelmfileRepos(helmfilePath string) ([]helmfileRepo, error) {
+	data, err := os.ReadFile(helmfilePath)
+	if err != nil {
+		return nil, fmt.Errorf("read helmfile %s: %w", helmfilePath, err)
+	}
+	return parseHelmfileReposBytes(data)
+}
+
+func parseHelmfileReposBytes(data []byte) ([]helmfileRepo, error) {
+	var doc helmfileDoc
+	if err := yaml.Unmarshal(data, &doc); err != nil {
+		return nil, fmt.Errorf("parse helmfile yaml: %w", err)
+	}
+
+	out := make([]helmfileRepo, 0, len(doc.Repositories))
+	for _, r := range doc.Repositories {
+		if strings.TrimSpace(r.Name) == "" || strings.TrimSpace(r.URL) == "" {
+			continue
+		}
+		out = append(out, r)
+	}
+	return out, nil
+}
+
+// ManagedRepoNames returns just the repo names from a helmfile.yaml. These are
+// the only repos this stack is responsible for keeping up to date; everything
+// else in the user's global `helm repo list` belongs to other tools.
+func ManagedRepoNames(helmfilePath string) ([]string, error) {
+	repos, err := ParseHelmfileRepos(helmfilePath)
+	if err != nil {
+		return nil, err
+	}
+	names := make([]string, 0, len(repos))
+	for _, r := range repos {
+		names = append(names, r.Name)
+	}
+	return names, nil
+}
+
+// EnsureRepos registers each (name, url) pair via `helm repo add --force-update`
+// so that a fresh host without `helm repo add` for our managed repos still gets
+// them registered before we ask helm to update them by name. Best-effort:
+// failures are returned for visibility but should not be treated as fatal by
+// callers (the subsequent `helm repo update` will surface real problems).
+func EnsureRepos(helmBinary string, repos []helmfileRepo) error {
+	var firstErr error
+	for _, r := range repos {
+		cmd := exec.Command(helmBinary, "repo", "add", "--force-update", r.Name, r.URL)
+		if out, err := cmd.CombinedOutput(); err != nil {
+			if firstErr == nil {
+				firstErr = fmt.Errorf("helm repo add %s %s: %w (%s)", r.Name, r.URL, err, strings.TrimSpace(string(out)))
+			}
+		}
+	}
+	return firstErr
+}
+
+// RepoUpdateSupportsFailOnRepoUpdateFail reports whether the current helm
+// binary accepts `helm repo update --fail-on-repo-update-fail=false`.
+//
+// Do not infer this from the major version. Some Helm 4 builds dropped the flag
+// even though Helm 3.14+ had it, and passing an unknown flag prevents the
+// targeted repo update from running at all.
+func RepoUpdateSupportsFailOnRepoUpdateFail(helmBinary string) bool {
+	cmd := exec.Command(helmBinary, "repo", "update", "--help")
+	out, err := cmd.CombinedOutput()
+	if err != nil {
+		return false
+	}
+	return strings.Contains(string(out), "--fail-on-repo-update-fail")
+}
+
+// UpdateRepos runs `helm repo update <names...>` and, when the helm binary
+// advertises support, passes --fail-on-repo-update-fail=false so that a single
+// dead repo doesn't abort the whole update.
+//
+// Behaviour:
+//   - helm versions that advertise --fail-on-repo-update-fail: the flag is
+//     passed and the returned error is nil even if individual repos in `names`
+//     fail.
+//   - other helm versions: the flag is omitted and the error surfaces normally.
+//
+// The targeted form (`helm repo update <names...>`) is important: it limits the
+// update to repos this stack actually needs, so unrelated dead repos in the
+// user's global helm config can't break us even on helm versions that lack the
+// tolerant flag.
+func UpdateRepos(helmBinary string, names []string) ([]byte, error) {
+	if len(names) == 0 {
+		return nil, nil
+	}
+	args := []string{"repo", "update"}
+	if RepoUpdateSupportsFailOnRepoUpdateFail(helmBinary) {
+		args = append(args, "--fail-on-repo-update-fail=false")
+	}
+	args = append(args, names...)
+
+	cmd := exec.Command(helmBinary, args...)
+	out, err := cmd.CombinedOutput()
+	return out, err
+}

internal/helmcmd/helmcmd_test.go

@@ -1,6 +1,11 @@
 package helmcmd
 
-import "testing"
+import (
+	"os"
+	"path/filepath"
+	"reflect"
+	"testing"
+)
 
 func TestParseMajor(t *testing.T) {
 	cases := []struct {
@@ -31,3 +36,203 @@ func TestParseMajor(t *testing.T) {
 		}
 	}
 }
+
+// TestParseHelmfileReposBytes is the table-driven happy/edge path for the
+// helmfile repo extractor that the tolerant-update preflight depends on.
+func TestParseHelmfileReposBytes(t *testing.T) {
+	cases := []struct {
+		name string
+		in   string
+		want []helmfileRepo
+	}{
+		{
+			name: "minimal valid helmfile with multiple repos",
+			in: `
+repositories:
+  - name: traefik
+    url: https://traefik.github.io/charts
+  - name: prometheus-community
+    url: https://prometheus-community.github.io/helm-charts
+releases:
+  - name: ignored
+    chart: ./ignored
+`,
+			want: []helmfileRepo{
+				{Name: "traefik", URL: "https://traefik.github.io/charts"},
+				{Name: "prometheus-community", URL: "https://prometheus-community.github.io/helm-charts"},
+			},
+		},
+		{
+			name: "entries missing name or url are skipped (OCI-only refs etc.)",
+			in: `
+repositories:
+  - name: traefik
+    url: https://traefik.github.io/charts
+  - name: oci-only
+  - url: https://nameless.example/charts
+`,
+			want: []helmfileRepo{
+				{Name: "traefik", URL: "https://traefik.github.io/charts"},
+			},
+		},
+		{
+			name: "no repositories key returns empty slice (not nil-failure)",
+			in: `
+releases: []
+`,
+			want: []helmfileRepo{},
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			got, err := parseHelmfileReposBytes([]byte(tc.in))
+			if err != nil {
+				t.Fatalf("parseHelmfileReposBytes: %v", err)
+			}
+			if !reflect.DeepEqual(got, tc.want) {
+				t.Fatalf("repos = %#v, want %#v", got, tc.want)
+			}
+		})
+	}
+}
+
+func TestParseHelmfileRepos_FromFile(t *testing.T) {
+	dir := t.TempDir()
+	path := filepath.Join(dir, "helmfile.yaml")
+	body := `
+repositories:
+  - name: obol
+    url: https://obolnetwork.github.io/helm-charts/
+`
+	if err := os.WriteFile(path, []byte(body), 0o600); err != nil {
+		t.Fatalf("write helmfile: %v", err)
+	}
+
+	repos, err := ParseHelmfileRepos(path)
+	if err != nil {
+		t.Fatalf("ParseHelmfileRepos: %v", err)
+	}
+	if len(repos) != 1 || repos[0].Name != "obol" {
+		t.Fatalf("unexpected repos: %#v", repos)
+	}
+
+	names, err := ManagedRepoNames(path)
+	if err != nil {
+		t.Fatalf("ManagedRepoNames: %v", err)
+	}
+	if !reflect.DeepEqual(names, []string{"obol"}) {
+		t.Fatalf("names = %#v, want [obol]", names)
+	}
+}
+
+// TestUpdateRepos_NoNamesIsNoop ensures we don't shell out for an empty repo
+// list — the preflight should silently do nothing rather than running
+// `helm repo update` with no args (which would update every globally
+// registered repo, defeating the whole point of the fix).
+func TestUpdateRepos_NoNamesIsNoop(t *testing.T) {
+	out, err := UpdateRepos("/nonexistent/helm", nil)
+	if err != nil {
+		t.Fatalf("UpdateRepos(nil) returned err: %v", err)
+	}
+	if out != nil {
+		t.Fatalf("UpdateRepos(nil) returned output: %q", out)
+	}
+}
+
+// TestUpdateRepos_TolerantArgsConstructed verifies that the tolerant flag is
+// passed when invoking against a real helm-3-style version response. We use a
+// fake helm binary written into the temp dir so the test stays hermetic. The
+// fake records its argv to a sentinel file so we can assert on it.
+func TestUpdateRepos_TolerantArgsConstructed(t *testing.T) {
+	if os.Getenv("GOOS") == "windows" {
+		t.Skip("shell-script fake binary not supported on windows")
+	}
+
+	dir := t.TempDir()
+	helm := filepath.Join(dir, "helm")
+	argLog := filepath.Join(dir, "args.log")
+
+	// Fake helm:
+	//   - `repo update --help` → advertises the tolerant flag
+	//   - any other args       → append to args.log and exit 0
+	script := `#!/bin/sh
+if [ "$1" = "repo" ] && [ "$2" = "update" ] && [ "$3" = "--help" ]; then
+  echo "      --fail-on-repo-update-fail=false   tolerate individual repo failures"
+  exit 0
+fi
+echo "$@" >> "` + argLog + `"
+exit 0
+`
+	if err := os.WriteFile(helm, []byte(script), 0o755); err != nil { //nolint:gosec // test binary
+		t.Fatalf("write fake helm: %v", err)
+	}
+
+	if _, err := UpdateRepos(helm, []string{"traefik", "obol"}); err != nil {
+		t.Fatalf("UpdateRepos: %v", err)
+	}
+
+	logged, err := os.ReadFile(argLog)
+	if err != nil {
+		t.Fatalf("read args log: %v", err)
+	}
+	got := string(logged)
+	for _, want := range []string{"repo", "update", "--fail-on-repo-update-fail=false", "traefik", "obol"} {
+		if !contains(got, want) {
+			t.Fatalf("expected %q in fake helm argv, got: %s", want, got)
+		}
+	}
+}
+
+// TestUpdateRepos_OmitsUnsupportedTolerantFlag covers Helm builds whose
+// `repo update` command no longer accepts --fail-on-repo-update-fail. The
+// preflight must still run a targeted repo update instead of failing before
+// any repo is refreshed.
+func TestUpdateRepos_OmitsUnsupportedTolerantFlag(t *testing.T) {
+	if os.Getenv("GOOS") == "windows" {
+		t.Skip("shell-script fake binary not supported on windows")
+	}
+
+	dir := t.TempDir()
+	helm := filepath.Join(dir, "helm")
+	argLog := filepath.Join(dir, "args.log")
+
+	script := `#!/bin/sh
+if [ "$1" = "repo" ] && [ "$2" = "update" ] && [ "$3" = "--help" ]; then
+  echo "Usage: helm repo update [REPO1 [REPO2 ...]] [flags]"
+  exit 0
+fi
+echo "$@" >> "` + argLog + `"
+exit 0
+`
+	if err := os.WriteFile(helm, []byte(script), 0o755); err != nil { //nolint:gosec // test binary
+		t.Fatalf("write fake helm: %v", err)
+	}
+
+	if _, err := UpdateRepos(helm, []string{"traefik", "obol"}); err != nil {
+		t.Fatalf("UpdateRepos: %v", err)
+	}
+
+	logged, err := os.ReadFile(argLog)
+	if err != nil {
+		t.Fatalf("read args log: %v", err)
+	}
+	got := string(logged)
+	if contains(got, "--fail-on-repo-update-fail=false") {
+		t.Fatalf("unsupported tolerant flag was passed: %s", got)
+	}
+	for _, want := range []string{"repo", "update", "traefik", "obol"} {
+		if !contains(got, want) {
+			t.Fatalf("expected %q in fake helm argv, got: %s", want, got)
+		}
+	}
+}
+
+func contains(haystack, needle string) bool {
+	for i := 0; i+len(needle) <= len(haystack); i++ {
+		if haystack[i:i+len(needle)] == needle {
+			return true
+		}
+	}
+	return false
+}

internal/stack/backend_k3d.go

@@ -107,10 +107,10 @@ func (b *K3dBackend) Up(cfg *config.Config, u *ui.UI, stackID string) ([]byte, e
 	// existing-cluster and fresh-create branches. `k3d cluster start` does
 	// not auto-restart standalone registry containers attached via
 	// `--registry-use` at create time — it only starts the cluster's own
-	// nodes. Without this call, every retry after a `cluster stop` (or after
-	// the failure-recovery Down() call in syncDefaults) falls back to direct
-	// upstream pulls and re-fetches every image, costing minutes per
-	// attempt.
+	// nodes. Without this call, every retry after a `cluster stop` (or any
+	// other path that left the registry containers in an `Exited` state)
+	// falls back to direct upstream pulls and re-fetches every image,
+	// costing minutes per attempt.
 	if os.Getenv("OBOL_DEVELOPMENT") == "true" {
 		setup, setupErr := ensureDevRegistries(cfg, u)
 		if setupErr != nil {