Repository navigation
Conversation
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 <lucasbarros.kam@gmail.com>
cpanato
requested changes
Nov 10, 2026
cpanato
left a comment
Member
There was a problem hiding this comment.
This addresses a real deadlock: the added large-stderr test times out against the original implementation, and local go test ./... passes with the fix. However, I reproduced two additional hangs introduced by the new pipe lifecycle. Please address the inline findings and add regression coverage before merging.
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 <lucasbarros.kam@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
RunProcesswraps stdout and stderr in anio.MultiReader, so stderr is only read after stdout reaches EOF. A command that writes more than a pipe buffer (64 KiB on Linux) to stderr while stdout is still open blocks on the write, andcthangs forever.helm install --debug,helm dependency buildorkubectl logswith a chatty container can hit this.Two more problems in the same function:
bufio.Scannerstops at lines longer than 64 KiB. The reader goroutine then exits and closes the pipes, and the command dies with SIGPIPE (failed waiting for process: signal: broken pipe).cmd.Wait()can run before the goroutine has finished reading, and theos/execdocs say Wait must not be called before all reads from the pipe are done, so the end of the output can be lost.This change passes the same writer as
StdoutandStderr, soos/execuses one pipe for both and copies it to stdout as is.WaitDelaystops reading one second after the command exits, so background processes that inherited the pipe cannot keepctwaiting, and a failed write to stdout closes the read end so the command gets EPIPE instead of blocking. Write errors are returned.pkg/exec/exec_test.gocovers 1 MiB on stderr before stdout, a 1 MiB line, a command that leaves a background process holding the pipe (both with exit 0 and a failing exit status), and a broken stdout with 2 MiB of output. Each case runs under a timeout.go test -race ./...and golangci-lint pass.Which issue this PR fixes: fixes #471
Special notes for your reviewer:
#471 was closed by the stale bot, but the cause is still in
RunProcess.AI disclosure: written with the help of an AI coding assistant, reviewed and tested by me.