From 6c18328c32052491e504542f5393011e7865a015 Mon Sep 17 00:00:00 2001 From: Lucas Barros Date: Fri, 6 Nov 2026 00:45:22 -0300 Subject: [PATCH 1/2] fix(exec): stop RunProcess from hanging on large stderr output RunProcess read stdout to EOF before reading stderr, so a command that wrote more than a pipe buffer to stderr blocked forever. The scanner also gave up on lines longer than 64KiB and closed the pipes, which killed the command with SIGPIPE. Write both streams to a single pipe, copy it to stdout as is, and only call Wait once the copy is done. Signed-off-by: Lucas Barros --- pkg/exec/exec.go | 29 +++++++++------------- pkg/exec/exec_test.go | 57 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 17 deletions(-) create mode 100644 pkg/exec/exec_test.go diff --git a/pkg/exec/exec.go b/pkg/exec/exec.go index 6a946c520..9eb5d08e9 100644 --- a/pkg/exec/exec.go +++ b/pkg/exec/exec.go @@ -15,7 +15,6 @@ package exec import ( - "bufio" "fmt" "io" "os" @@ -79,30 +78,26 @@ func (p ProcessExecutor) RunProcess(executable string, execArgs ...interface{}) return err } - outReader, err := cmd.StdoutPipe() + // Send stdout and stderr to the same pipe so that neither can fill up + // while the other one is being read. + reader, writer, err := os.Pipe() if err != nil { - return fmt.Errorf("failed getting StdoutPipe for command: %w", err) + return fmt.Errorf("failed creating pipe for command: %w", err) } - - errReader, err := cmd.StderrPipe() - if err != nil { - return fmt.Errorf("failed getting StderrPipe for command: %w", err) - } - - scanner := bufio.NewScanner(io.MultiReader(outReader, errReader)) - go func() { - defer outReader.Close() - defer errReader.Close() - for scanner.Scan() { - fmt.Println(scanner.Text()) - } - }() + defer reader.Close() + cmd.Stdout = writer + cmd.Stderr = writer err = cmd.Start() + writer.Close() if err != nil { return fmt.Errorf("failed running process: %w", err) } + // Copy the output before waiting for the process; Wait must not be + // called before all reads from the pipe have completed. + _, _ = io.Copy(os.Stdout, reader) + err = cmd.Wait() if err != nil { return fmt.Errorf("failed waiting for process: %w", err) diff --git a/pkg/exec/exec_test.go b/pkg/exec/exec_test.go new file mode 100644 index 000000000..093dcd418 --- /dev/null +++ b/pkg/exec/exec_test.go @@ -0,0 +1,57 @@ +// 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 ( + "testing" + "time" +) + +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`, + }, + } + + p := NewProcessExecutor(false) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + errCh := make(chan error, 1) + go func() { + errCh <- p.RunProcess("sh", "-c", tt.script) + }() + + select { + case err := <-errCh: + if err != nil { + t.Fatalf("RunProcess returned an error: %v", err) + } + case <-time.After(30 * time.Second): + t.Fatal("RunProcess did not return") + } + }) + } +} From 75d6ddec842fb2d8105697ec94ea8adf2e5ce529 Mon Sep 17 00:00:00 2001 From: Lucas Barros Date: Tue, 10 Nov 2026 13:42:23 -0300 Subject: [PATCH 2/2] fix(exec): bound output draining and handle write errors in RunProcess Reading the pipe to EOF before calling Wait also waited for background processes that inherited stdout and stderr, so a command that leaves a service running kept ct blocked. If writing to stdout failed, the copy stopped but the read end stayed open, and a command writing more than a pipe buffer blocked before Wait could return. Let exec own the pipe instead: passing the same non-file writer as Stdout and Stderr still gives one shared pipe, WaitDelay stops reading one second after the command exits, and a failed write closes the read end so the command gets EPIPE. Write errors are returned to the caller. Signed-off-by: Lucas Barros --- pkg/exec/exec.go | 53 +++++++++++++------- pkg/exec/exec_test.go | 113 +++++++++++++++++++++++++++++++++++++----- 2 files changed, 136 insertions(+), 30 deletions(-) diff --git a/pkg/exec/exec.go b/pkg/exec/exec.go index 9eb5d08e9..73fe8721a 100644 --- a/pkg/exec/exec.go +++ b/pkg/exec/exec.go @@ -15,11 +15,13 @@ package exec import ( + "errors" "fmt" "io" "os" "os/exec" "strings" + "time" "github.com/helm/chart-testing/v3/pkg/util" ) @@ -72,34 +74,51 @@ func (p ProcessExecutor) RunProcessInDirAndCaptureStdout(workingDirectory string return strings.TrimSpace(string(bytes)), nil } +// 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 +} + +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 +} + func (p ProcessExecutor) RunProcess(executable string, execArgs ...interface{}) error { cmd, err := p.CreateProcess(executable, execArgs...) if err != nil { return err } - // Send stdout and stderr to the same pipe so that neither can fill up - // while the other one is being read. - reader, writer, err := os.Pipe() - if err != nil { - return fmt.Errorf("failed creating pipe for command: %w", err) - } - defer reader.Close() - cmd.Stdout = writer - cmd.Stderr = writer + // 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() - writer.Close() - if err != nil { + if err := cmd.Start(); err != nil { return fmt.Errorf("failed running process: %w", err) } - // Copy the output before waiting for the process; Wait must not be - // called before all reads from the pipe have completed. - _, _ = io.Copy(os.Stdout, reader) - 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) } diff --git a/pkg/exec/exec_test.go b/pkg/exec/exec_test.go index 093dcd418..d4b45081f 100644 --- a/pkg/exec/exec_test.go +++ b/pkg/exec/exec_test.go @@ -15,10 +15,36 @@ 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 @@ -36,22 +62,83 @@ func TestRunProcessDoesNotBlock(t *testing.T) { }, } - p := NewProcessExecutor(false) for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - errCh := make(chan error, 1) - go func() { - errCh <- p.RunProcess("sh", "-c", tt.script) - }() - - select { - case err := <-errCh: - if err != nil { - t.Fatalf("RunProcess returned an error: %v", err) - } - case <-time.After(30 * time.Second): - t.Fatal("RunProcess did not return") + 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) + } +}