fix(exec): stop RunProcess from hanging on large stderr output

pkg/exec/exec.go

@@ -15,12 +15,13 @@
 package exec
 
 import (
-	"bufio"
+	"errors"
 	"fmt"
 	"io"
 	"os"
 	"os/exec"
 	"strings"
+	"time"
 
 	"github.com/helm/chart-testing/v3/pkg/util"
 )
@@ -73,38 +74,51 @@ func (p ProcessExecutor) RunProcessInDirAndCaptureStdout(workingDirectory string
 	return strings.TrimSpace(string(bytes)), nil
 }
 
-func (p ProcessExecutor) RunProcess(executable string, execArgs ...interface{}) error {
-	cmd, err := p.CreateProcess(executable, execArgs...)
-	if err != nil {
-		return err
-	}
+// outputDrainTimeout bounds how long RunProcess keeps reading output after
+// the command has exited. Background processes started by the command inherit
+// its stdout and stderr and can keep the pipe open indefinitely.
+const outputDrainTimeout = time.Second
+
+// processOutput forwards the combined stdout and stderr of a command and
+// remembers the first write error. It is deliberately not an *os.File, so
+// exec copies the output through a pipe that Wait closes.
+type processOutput struct {
+	w   io.Writer
+	err error
+}
 
-	outReader, err := cmd.StdoutPipe()
-	if err != nil {
-		return fmt.Errorf("failed getting StdoutPipe for command: %w", err)
+func (o *processOutput) Write(b []byte) (int, error) {
+	n, err := o.w.Write(b)
+	if err != nil && o.err == nil {
+		o.err = err
 	}
+	return n, err
+}
 
-	errReader, err := cmd.StderrPipe()
+func (p ProcessExecutor) RunProcess(executable string, execArgs ...interface{}) error {
+	cmd, err := p.CreateProcess(executable, execArgs...)
 	if err != nil {
-		return fmt.Errorf("failed getting StderrPipe for command: %w", err)
+		return err
 	}
 
-	scanner := bufio.NewScanner(io.MultiReader(outReader, errReader))
-	go func() {
-		defer outReader.Close()
-		defer errReader.Close()
-		for scanner.Scan() {
-			fmt.Println(scanner.Text())
-		}
-	}()
+	// Using the same writer for stdout and stderr makes exec send both to a
+	// single pipe, so neither can fill up while the other one is being read.
+	// If writing fails, exec closes the read end and the command gets EPIPE
+	// instead of blocking on a full pipe.
+	out := &processOutput{w: os.Stdout}
+	cmd.Stdout = out
+	cmd.Stderr = out
+	cmd.WaitDelay = outputDrainTimeout
 
-	err = cmd.Start()
-	if err != nil {
+	if err := cmd.Start(); err != nil {
 		return fmt.Errorf("failed running process: %w", err)
 	}
 
 	err = cmd.Wait()
-	if err != nil {
+	if out.err != nil {
+		return fmt.Errorf("failed writing process output: %w", out.err)
+	}
+	if err != nil && !errors.Is(err, exec.ErrWaitDelay) {
 		return fmt.Errorf("failed waiting for process: %w", err)
 	}
 

pkg/exec/exec_test.go

@@ -0,0 +1,144 @@
+// Copyright The Helm Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package exec
+
+import (
+	"errors"
+	"os"
+	"path/filepath"
+	"strconv"
+	"strings"
+	"syscall"
+	"testing"
+	"time"
+)
+
+// runProcess runs a shell script through RunProcess and fails the test if it
+// does not return within the timeout.
+func runProcess(t *testing.T, timeout time.Duration, script string) error {
+	t.Helper()
+
+	p := NewProcessExecutor(false)
+	errCh := make(chan error, 1)
+	go func() {
+		errCh <- p.RunProcess("sh", "-c", script)
+	}()
+
+	select {
+	case err := <-errCh:
+		return err
+	case <-time.After(timeout):
+		t.Fatal("RunProcess did not return")
+		return nil
+	}
+}
+
+func TestRunProcessDoesNotBlock(t *testing.T) {
+	tests := []struct {
+		name   string
+		script string
+	}{
+		{
+			// More stderr than a pipe buffer holds, written before any stdout.
+			name:   "large stderr",
+			script: `yes warning | head -n 200000 >&2; echo done`,
+		},
+		{
+			// A single line longer than bufio.Scanner's default limit.
+			name:   "long line",
+			script: `head -c 1048576 /dev/zero | tr '\0' 'x'; echo; echo done`,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if err := runProcess(t, 30*time.Second, tt.script); err != nil {
+				t.Fatalf("RunProcess returned an error: %v", err)
+			}
+		})
+	}
+}
+
+// startBackgroundSleep returns a script fragment that starts a long sleep in
+// the background, inheriting the command's stdout and stderr, and kills it
+// when the test ends.
+func startBackgroundSleep(t *testing.T) string {
+	t.Helper()
+
+	pidFile := filepath.Join(t.TempDir(), "pid")
+	t.Cleanup(func() {
+		b, err := os.ReadFile(pidFile)
+		if err != nil {
+			return
+		}
+		if pid, err := strconv.Atoi(strings.TrimSpace(string(b))); err == nil {
+			_ = syscall.Kill(pid, syscall.SIGKILL)
+		}
+	})
+	return "sleep 60 & echo $! > " + pidFile + ";"
+}
+
+func TestRunProcessDoesNotWaitForBackgroundChildren(t *testing.T) {
+	tests := []struct {
+		name    string
+		script  string
+		wantErr bool
+	}{
+		{
+			name:   "command succeeds",
+			script: "echo started",
+		},
+		{
+			name:    "command fails",
+			script:  "exit 3",
+			wantErr: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			script := startBackgroundSleep(t) + tt.script
+			start := time.Now()
+			err := runProcess(t, 20*time.Second, script)
+			if tt.wantErr != (err != nil) {
+				t.Fatalf("RunProcess returned error %v, want error: %t", err, tt.wantErr)
+			}
+			if elapsed := time.Since(start); elapsed > 10*time.Second {
+				t.Fatalf("RunProcess took %s, it waited for the background child", elapsed)
+			}
+		})
+	}
+}
+
+func TestRunProcessReturnsWhenStdoutIsBroken(t *testing.T) {
+	reader, writer, err := os.Pipe()
+	if err != nil {
+		t.Fatal(err)
+	}
+	reader.Close()
+	stdout := os.Stdout
+	os.Stdout = writer
+	t.Cleanup(func() {
+		os.Stdout = stdout
+		writer.Close()
+	})
+
+	// Much more than a pipe buffer holds, so the command blocks if nothing
+	// drains or closes its end of the pipe.
+	err = runProcess(t, 20*time.Second, `head -c 2097152 /dev/zero`)
+	if !errors.Is(err, syscall.EPIPE) {
+		t.Fatalf("RunProcess returned error %v, want the broken pipe error from writing its output", err)
+	}
+}