fix: better handling for transient failures

chartvalidator/checker/engine_app_checker.go

@@ -41,14 +41,15 @@ func NewAppCheckerEngine(context context.Context, outputDir string, apiVersions
 	errorChan := make(chan ErrorResult)
 
 	cre := ChartRenderingEngine{
-		inputChan:   make(chan ChartRenderParams),
-		resultChan:  make(chan RenderResult),
-		errorChan:   errorChan,
-		outputDir:   outputDir,
-		context:     context,
-		executor:    &RealCommandExecutor{},
-		name:        "ChartRenderer",
-		apiVersions: apiVersions,
+		inputChan:    make(chan ChartRenderParams),
+		resultChan:   make(chan RenderResult),
+		errorChan:    errorChan,
+		outputDir:    outputDir,
+		context:      context,
+		executor:     &RealCommandExecutor{},
+		name:         "ChartRenderer",
+		retrySleepFn: time.Sleep,
+		apiVersions:  apiVersions,
 	}
 
 	// Construct absolute path to schemas directory

chartvalidator/checker/engine_chart_rendering.go

@@ -9,6 +9,7 @@ import (
 	"path/filepath"
 	"strings"
 	"sync"
+	"time"
 )
 
 type ChartRenderingEngine struct {
@@ -20,6 +21,7 @@ type ChartRenderingEngine struct {
 	context         context.Context
 	executor        CommandExecutor
 	name            string
+	retrySleepFn    func(time.Duration)
 	workerWaitGroup sync.WaitGroup
 	apiVersions     []string
 }
@@ -117,17 +119,9 @@ func (engine *ChartRenderingEngine) renderSingleChart(chart ChartRenderParams, w
 	}
 
 	logEngineDebug(engine.name, workerId, fmt.Sprintf("helm %s", strings.Join(args, " ")))
-	cmd := engine.executor.CommandContext(engine.context, "helm", args...)
 
-	// Set working directory to current directory so relative paths work
-	if wd, err := os.Getwd(); err == nil {
-		cmd.SetDir(wd)
-	}
-
-	output, err := cmd.CombinedOutput()
+	output, err := engine.runHelm(args, workerId)
 	if err != nil {
-		msg := fmt.Sprintf("helm command failed: %s\nOutput: %s", err.Error(), string(output))
-		logEngineWarning(engine.name, workerId, msg)
 		return nil, fmt.Errorf("helm command failed: %w", err)
 	}
 
@@ -157,6 +151,100 @@ func (engine *ChartRenderingEngine) renderSingleChart(chart ChartRenderParams, w
 	return &RenderResult{Chart: chart, ManifestPath: outputPath}, nil
 }
 
+// runHelm executes `helm` and retries when the failure looks like a network
+// problem. Every render of a non-OCI chart downloads the repository index
+// again, so a run with many charts opens many connections to the same host at
+// once. A host such as GitHub Pages answers some of them with a TCP reset,
+// which fails the whole pipeline although nothing is wrong with the chart.
+func (engine *ChartRenderingEngine) runHelm(args []string, workerId int) ([]byte, error) {
+	const maxRetries = 3
+
+	cmdStr := fmt.Sprintf("helm %s", strings.Join(args, " "))
+
+	var output []byte
+	var err error
+	for attempt := 0; attempt <= maxRetries; attempt++ {
+		if attempt > 0 {
+			sleepSecs := 1 + rand.Intn(15)
+			logEngineWarning(engine.name, workerId, fmt.Sprintf("retrying %s (attempt %d/%d) after %ds", cmdStr, attempt+1, maxRetries+1, sleepSecs))
+			engine.sleep(time.Duration(sleepSecs) * time.Second)
+			select {
+			case <-engine.context.Done():
+				return nil, engine.context.Err()
+			default:
+			}
+		}
+
+		cmd := engine.executor.CommandContext(engine.context, "helm", args...)
+
+		// Set working directory to current directory so relative paths work
+		if wd, wdErr := os.Getwd(); wdErr == nil {
+			cmd.SetDir(wd)
+		}
+
+		output, err = cmd.CombinedOutput()
+		if err == nil {
+			return output, nil
+		}
+
+		msg := fmt.Sprintf("helm command failed: %s\nOutput: %s", err.Error(), string(output))
+		logEngineWarning(engine.name, workerId, msg)
+
+		if !isTransientHelmError(output) {
+			logEngineWarning(engine.name, workerId, fmt.Sprintf("not retrying %s: permanent failure detected", cmdStr))
+			break
+		}
+	}
+
+	return output, err
+}
+
+// sleep waits for the given duration. A test replaces retrySleepFn to keep the
+// retry path fast; an engine built without one falls back to time.Sleep.
+func (engine *ChartRenderingEngine) sleep(d time.Duration) {
+	if engine.retrySleepFn != nil {
+		engine.retrySleepFn(d)
+		return
+	}
+	time.Sleep(d)
+}
+
+// isTransientHelmError returns true if the command output indicates a failure
+// that can resolve on retry, such as a reset connection or a 5xx answer from
+// the chart repository. The test is the opposite of the one the Docker engine
+// makes: most helm template failures are permanent (a bad value, a broken
+// template, a missing chart version), so this list names what to retry instead
+// of what to give up on.
+func isTransientHelmError(output []byte) bool {
+	s := strings.ToLower(string(output))
+	for _, pattern := range []string{
+		"connection reset by peer",
+		"connection refused",
+		"broken pipe",
+		"unexpected eof",
+		"i/o timeout",
+		"context deadline exceeded",
+		"client.timeout",
+		"tls handshake",
+		"network is unreachable",
+		"no route to host",
+		"temporary failure in name resolution",
+		"server misbehaving",
+		"no such host",
+		"too many requests",
+		"429 ",
+		"500 internal server error",
+		"502 bad gateway",
+		"503 service unavailable",
+		"504 gateway",
+	} {
+		if strings.Contains(s, pattern) {
+			return true
+		}
+	}
+	return false
+}
+
 func isOCIRepo(repoURL string) bool {
 	return strings.HasPrefix(repoURL, "oci://")
 }

chartvalidator/checker/engine_chart_rendering_test.go

@@ -2,21 +2,24 @@ package main
 
 import (
 	"context"
+	"fmt"
 	"os"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/assert"
 )
 
 // Helper function to create and start a chart rendering engine
 func createEngine(mockExecutor *MockCommandExecutor, includeErrorChan bool) *ChartRenderingEngine {
 	engine := &ChartRenderingEngine{
-		inputChan:   make(chan ChartRenderParams),
-		resultChan:  make(chan RenderResult),
-		outputDir:   "test_output",
-		context:     context.Background(),
-		executor:    mockExecutor,
-		apiVersions: []string{"something", "something-else"},
+		inputChan:    make(chan ChartRenderParams),
+		resultChan:   make(chan RenderResult),
+		outputDir:    "test_output",
+		context:      context.Background(),
+		executor:     mockExecutor,
+		retrySleepFn: func(time.Duration) {},
+		apiVersions:  []string{"something", "something-else"},
 	}
 
 	if includeErrorChan {
@@ -136,3 +139,86 @@ func TestRenderBaseFileNotExist(t *testing.T) {
 	assert.NotNil(t, errorResult.Error)
 	assert.Contains(t, errorResult.Error.Error(), "base values file does not exist")
 }
+
+// The real message helm prints when GitHub Pages resets the connection while
+// the repository index downloads.
+const helmResetOutput = `Error: looks like "https://interledger.github.io/charts/interledger" is not a valid chart repository or cannot be reached: Get "https://interledger.github.io/charts/interledger/index.yaml": read tcp 172.18.0.2:49324->185.199.108.153:443: read: connection reset by peer`
+
+func TestRenderRetriesTransientError(t *testing.T) {
+	callCount := 0
+	mockExecutor := createMockExecutorWithBehavior(func() error {
+		callCount++
+		if callCount < 3 {
+			return fmt.Errorf("exit status 1")
+		}
+		return nil
+	})
+	mockExecutor.Output = []byte(helmResetOutput)
+
+	engine := createEngine(mockExecutor, true)
+	defer cleanupEngine(engine)
+
+	engine.inputChan <- createTestChart()
+
+	result := <-engine.resultChan
+	assert.Equal(t, "test-chart", result.Chart.ChartName)
+	assert.Equal(t, 3, callCount, "expected 3 attempts (2 failures then 1 success)")
+}
+
+func TestRenderRetriesExhausted(t *testing.T) {
+	callCount := 0
+	mockExecutor := createMockExecutorWithBehavior(func() error {
+		callCount++
+		return fmt.Errorf("exit status 1")
+	})
+	mockExecutor.Output = []byte(helmResetOutput)
+
+	engine := createEngine(mockExecutor, true)
+	defer cleanupEngine(engine)
+
+	engine.inputChan <- createTestChart()
+
+	errResult := <-engine.errorChan
+	assert.NotNil(t, errResult.Error)
+	assert.Equal(t, 4, callCount, "expected 4 attempts (1 initial + 3 retries)")
+}
+
+func TestRenderDoesNotRetryPermanentError(t *testing.T) {
+	callCount := 0
+	mockExecutor := createMockExecutorWithBehavior(func() error {
+		callCount++
+		return fmt.Errorf("exit status 1")
+	})
+	mockExecutor.Output = []byte(`Error: chart "rafiki" version "3.2.0" not found in https://interledger.github.io/charts/interledger repository`)
+
+	engine := createEngine(mockExecutor, true)
+	defer cleanupEngine(engine)
+
+	engine.inputChan <- createTestChart()
+
+	errResult := <-engine.errorChan
+	assert.NotNil(t, errResult.Error)
+	assert.Equal(t, 1, callCount, "a missing chart version must not be retried")
+}
+
+func TestIsTransientHelmError(t *testing.T) {
+	transient := []string{
+		helmResetOutput,
+		`Get "https://example.com/index.yaml": net/http: TLS handshake timeout`,
+		`Get "https://example.com/index.yaml": dial tcp: i/o timeout`,
+		"Error: failed to fetch https://example.com/index.yaml : 503 Service Unavailable",
+	}
+	for _, output := range transient {
+		assert.True(t, isTransientHelmError([]byte(output)), "expected transient: %s", output)
+	}
+
+	permanent := []string{
+		`Error: chart "rafiki" version "3.2.0" not found`,
+		"Error: execution error at (rafiki/templates/deployment.yaml:12:3): image tag is required",
+		"Error: unauthorized",
+		"Error: YAML parse error on rafiki/values.yaml",
+	}
+	for _, output := range permanent {
+		assert.False(t, isTransientHelmError([]byte(output)), "expected permanent: %s", output)
+	}
+}