From f15d8d8ce09877023714db6565fc8d93709cc35d Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Tue, 1 Sep 2026 11:46:54 +0000 Subject: [PATCH] Preserve subprocess wait failures --- internal/adapters/subprocess/execution.go | 19 +++---- .../adapters/subprocess/execution_test.go | 54 +++++++++++++++++++ 2 files changed, 64 insertions(+), 9 deletions(-) create mode 100644 internal/adapters/subprocess/execution_test.go diff --git a/internal/adapters/subprocess/execution.go b/internal/adapters/subprocess/execution.go index d526f4d..62c8188 100644 --- a/internal/adapters/subprocess/execution.go +++ b/internal/adapters/subprocess/execution.go @@ -2,6 +2,7 @@ package subprocess import ( "context" + "fmt" "os/exec" "time" ) @@ -58,10 +59,10 @@ func executeOwnedCommand(ctx context.Context, cmd *exec.Cmd, abort <-chan error) } else { outcome.reason = executionCanceled } - outcome.cleanupErr = terminateAndReap(tree, waitCh) + outcome.waitErr, outcome.cleanupErr = terminateAndReap(tree, waitCh) case outcome.abortErr = <-abort: outcome.reason = executionAborted - outcome.cleanupErr = terminateAndReap(tree, waitCh) + outcome.waitErr, outcome.cleanupErr = terminateAndReap(tree, waitCh) } if cmd.ProcessState != nil { outcome.exitCode = cmd.ProcessState.ExitCode() @@ -69,22 +70,22 @@ func executeOwnedCommand(ctx context.Context, cmd *exec.Cmd, abort <-chan error) return outcome } -func terminateAndReap(tree ownedProcessTree, waitCh <-chan error) error { - cleanupErr := tree.TerminateGracefully() +func terminateAndReap(tree ownedProcessTree, waitCh <-chan error) (waitErr, cleanupErr error) { + cleanupErr = tree.TerminateGracefully() timer := time.NewTimer(gracefulTerminationWait) defer timer.Stop() select { - case <-waitCh: - return joinErrors(cleanupErr, tree.TerminateForcefully()) + case waitErr = <-waitCh: + return waitErr, joinErrors(cleanupErr, tree.TerminateForcefully()) case <-timer.C: } cleanupErr = joinErrors(cleanupErr, tree.TerminateForcefully()) forcefulTimer := time.NewTimer(forcefulTerminationWait) defer forcefulTimer.Stop() select { - case <-waitCh: - return cleanupErr + case waitErr = <-waitCh: + return waitErr, cleanupErr case <-forcefulTimer.C: - return joinErrors(cleanupErr, context.DeadlineExceeded) + return nil, joinErrors(cleanupErr, fmt.Errorf("owned subprocess did not reap within %s after forceful termination", forcefulTerminationWait)) } } diff --git a/internal/adapters/subprocess/execution_test.go b/internal/adapters/subprocess/execution_test.go new file mode 100644 index 0000000..aa8eb34 --- /dev/null +++ b/internal/adapters/subprocess/execution_test.go @@ -0,0 +1,54 @@ +package subprocess + +import ( + "errors" + "os/exec" + "testing" +) + +func TestTerminateAndReapPreservesWaitAndCleanupErrors(t *testing.T) { + waitErr := errors.New("wait failed") + gracefulErr := errors.New("graceful termination failed") + forcefulErr := errors.New("forceful termination failed") + tree := &recordingProcessTree{ + gracefulErr: gracefulErr, + forcefulErr: forcefulErr, + } + waitCh := make(chan error, 1) + waitCh <- waitErr + + gotWaitErr, gotCleanupErr := terminateAndReap(tree, waitCh) + if !errors.Is(gotWaitErr, waitErr) { + t.Fatalf("wait error = %v, want %v", gotWaitErr, waitErr) + } + for _, want := range []error{gracefulErr, forcefulErr} { + if !errors.Is(gotCleanupErr, want) { + t.Fatalf("cleanup error = %v, want wrapped %v", gotCleanupErr, want) + } + } + if tree.gracefulCalls != 1 || tree.forcefulCalls != 1 { + t.Fatalf("termination calls = graceful %d, forceful %d; want 1 each", tree.gracefulCalls, tree.forcefulCalls) + } +} + +type recordingProcessTree struct { + gracefulErr error + forcefulErr error + + gracefulCalls int + forcefulCalls int +} + +func (*recordingProcessTree) Start(*exec.Cmd) error { return nil } + +func (tree *recordingProcessTree) TerminateGracefully() error { + tree.gracefulCalls++ + return tree.gracefulErr +} + +func (tree *recordingProcessTree) TerminateForcefully() error { + tree.forcefulCalls++ + return tree.forcefulErr +} + +func (*recordingProcessTree) Dispose() error { return nil }