From 9c02ed769bb8c7530540c76b8726464f4af4da6d Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Mon, 2 Nov 2026 03:14:59 +0000 Subject: [PATCH] Bound subprocess output teardown --- docs/roadmap/implementation.md | 2 +- internal/build/build.go | 16 +++--- internal/process/process.go | 2 + internal/process/process_test.go | 91 ++++++++++++++++++++++++++++++++ 4 files changed, 101 insertions(+), 10 deletions(-) diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md index b07a1dc..2990429 100644 --- a/docs/roadmap/implementation.md +++ b/docs/roadmap/implementation.md @@ -39,7 +39,7 @@ the agent policy; an intermediate compilation dependency is recorded accurately. | 1 | A01; publication assertion portion of A19 | Complete — regression failed before the fix and passed afterward; focused publication and reconciliation tests passed. | | 2 | A02, A12: common Git mechanics | Complete — focused Git, candidate, identity, and public build/publication/delivery tests passed; A02 regression evidence follows in Stage 3. | | 3 | A02: source identity regression evidence | Complete — commit, tree, and blob replacement regressions and public publication, build, and delivery cases passed. | -| 4 | A03: buffered command teardown | Pending | +| 4 | A03: buffered command teardown | Complete — inherited-pipe, cancellation, healthy-command, native build, and publication recovery checks passed. | | 5 | A03, A18: piped lifetime and queued cancellation | Pending | | 6 | A04 | Pending | | 7 | A05 | Pending | diff --git a/internal/build/build.go b/internal/build/build.go index ef82a70..28b25f2 100644 --- a/internal/build/build.go +++ b/internal/build/build.go @@ -2,6 +2,7 @@ package build import ( + "bytes" "context" "crypto/sha256" "encoding/hex" @@ -9,7 +10,6 @@ import ( "fmt" "io" "os" - "os/exec" "path/filepath" "strings" @@ -17,6 +17,7 @@ import ( "go.maximumdirect.net/releasekit/internal/candidate" "go.maximumdirect.net/releasekit/internal/git" "go.maximumdirect.net/releasekit/internal/gotool" + "go.maximumdirect.net/releasekit/internal/process" "go.maximumdirect.net/releasekit/internal/releaseidentity" "go.maximumdirect.net/releasekit/internal/tempdir" "go.maximumdirect.net/releasekit/internal/versioncontract" @@ -227,17 +228,14 @@ func hashArtifact(artifact *Artifact) error { } func checkVersion(ctx context.Context, binaryPath, binaryName, version string) error { - command := exec.CommandContext(ctx, binaryPath, "version") - output, err := command.CombinedOutput() + var output bytes.Buffer + err := process.Run(ctx, process.Invocation{Args: []string{binaryPath, "version"}, Stdout: &output, Stderr: &output}) if err != nil { - if ctx.Err() != nil { - return fmt.Errorf("run native version check: %w", ctx.Err()) - } - return fmt.Errorf("run native version check: %w: %s", err, strings.TrimSpace(string(output))) + return fmt.Errorf("run native version check: %w: %s", err, strings.TrimSpace(output.String())) } want := binaryName + " " + version + "\n" - if string(output) != want { - return fmt.Errorf("native version output = %q, want %q", output, want) + if output.String() != want { + return fmt.Errorf("native version output = %q, want %q", output.String(), want) } return nil } diff --git a/internal/process/process.go b/internal/process/process.go index 052d609..7839d49 100644 --- a/internal/process/process.go +++ b/internal/process/process.go @@ -11,6 +11,7 @@ import ( "sort" "strings" "sync" + "time" ) // Invocation describes one child process. UnsetEnv removes inherited values @@ -135,6 +136,7 @@ func newCommand(ctx context.Context, invocation Invocation) (*exec.Cmd, error) { } command := exec.CommandContext(ctx, invocation.Args[0], invocation.Args[1:]...) command.Dir = invocation.Dir + command.WaitDelay = time.Second if len(invocation.Env) != 0 || len(invocation.UnsetEnv) != 0 { command.Env = environmentEntries(invocation.Env, invocation.UnsetEnv) } diff --git a/internal/process/process_test.go b/internal/process/process_test.go index d071842..6d54cf1 100644 --- a/internal/process/process_test.go +++ b/internal/process/process_test.go @@ -10,6 +10,7 @@ import ( "os" "os/exec" "path/filepath" + "strconv" "strings" "testing" "time" @@ -119,6 +120,68 @@ func TestRunReturnsCancellation(t *testing.T) { } } +func TestRunBoundsInheritedOutputPipes(t *testing.T) { + for _, mode := range []string{"cancel-descendant", "exit-descendant"} { + t.Run(mode, func(t *testing.T) { + ready := filepath.Join(t.TempDir(), "descendant") + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + var output bytes.Buffer + result := make(chan error, 1) + go func() { + result <- Run(ctx, Invocation{ + Args: []string{helperExecutable(t), "-test.run=TestProcessHelper", "--", "unused", mode}, + Env: map[string]string{"RELEASEKIT_PROCESS_HELPER": "1", "RELEASEKIT_DESCENDANT_READY": ready}, + Stdout: &output, Stderr: &output, + }) + }() + deadline := time.After(10 * time.Second) + var pid int + for pid == 0 { + data, err := os.ReadFile(ready) + if err == nil { + pid, _ = strconv.Atoi(string(data)) + } + select { + case <-deadline: + t.Fatal("descendant did not become ready") + default: + time.Sleep(10 * time.Millisecond) + } + } + t.Cleanup(func() { + if process, err := os.FindProcess(pid); err == nil { + _ = process.Kill() + } + }) + if mode == "cancel-descendant" { + cancel() + } + select { + case err := <-result: + if mode == "cancel-descendant" && !errors.Is(err, context.Canceled) { + t.Fatalf("canceled command error = %v", err) + } + if mode == "exit-descendant" && !errors.Is(err, exec.ErrWaitDelay) { + t.Fatalf("exited command error = %v, want pipe teardown error", err) + } + case <-time.After(10 * time.Second): + t.Fatal("command did not release inherited output pipes") + } + }) + } +} + +func TestRunDoesNotLimitHealthyCommandLifetime(t *testing.T) { + err := Run(context.Background(), Invocation{ + Args: []string{helperExecutable(t), "-test.run=TestProcessHelper", "--", "unused", "slow-success"}, + Env: map[string]string{"RELEASEKIT_PROCESS_HELPER": "1"}, + }) + if err != nil { + t.Fatalf("healthy command failed: %v", err) + } +} + func TestStartPipedExchangesBeforeInputEOFAndPreservesSettings(t *testing.T) { workdir := filepath.Join(t.TempDir(), "piped directory") if err := os.Mkdir(workdir, 0o700); err != nil { @@ -279,11 +342,39 @@ func TestProcessHelper(t *testing.T) { os.Exit(0) case "exit": os.Exit(7) + case "slow-success": + time.Sleep(1500 * time.Millisecond) + os.Exit(0) case "wait": fmt.Fprintln(os.Stdout, "started") for { time.Sleep(time.Second) } + case "cancel-descendant", "exit-descendant": + child := exec.Command(os.Args[0], "-test.run=TestProcessHelper", "--", "unused", "descendant") + child.Stdout, child.Stderr = os.Stdout, os.Stderr + if err := child.Start(); err != nil { + os.Exit(14) + } + for { + if _, err := os.Stat(os.Getenv("RELEASEKIT_DESCENDANT_READY")); err == nil { + break + } + time.Sleep(10 * time.Millisecond) + } + if mode == "exit-descendant" { + os.Exit(0) + } + for { + time.Sleep(time.Second) + } + case "descendant": + if err := os.WriteFile(os.Getenv("RELEASEKIT_DESCENDANT_READY"), []byte(strconv.Itoa(os.Getpid())), 0o600); err != nil { + os.Exit(15) + } + for { + time.Sleep(time.Second) + } case "exchange": reader := bufio.NewReader(os.Stdin) request, err := reader.ReadString('\n')