From 50d35732576bbfdd043117f1022b55d69d13cffe Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Fri, 9 Oct 2026 01:44:33 +0000 Subject: [PATCH] Transfer completed Go output without copies --- docs/internal/gotool.md | 22 +++++ docs/roadmap/implementation.md | 20 ++++- internal/build/build.go | 4 +- internal/gotool/gotool.go | 66 ++++++++++++--- internal/gotool/gotool_test.go | 127 +++++++++++++++++++++++++++- internal/gotool/performance_test.go | 2 +- internal/validation/validation.go | 6 +- 7 files changed, 226 insertions(+), 21 deletions(-) diff --git a/docs/internal/gotool.md b/docs/internal/gotool.md index dd2ebff..dc7d575 100644 --- a/docs/internal/gotool.md +++ b/docs/internal/gotool.md @@ -5,6 +5,28 @@ operation and applies the shared environment to its Go commands. It uses `internal/process` for cancellation, the working directory, output, and wrapped errors. +## Command output ownership + +`Tool.Run` executes a status-only command. It forwards combined stdout and +stderr to the optional writer while the child runs, then releases its capture +on success. Failures include the captured diagnostic text and preserve the +underlying process or forwarding error. + +`Tool.Output` returns combined stdout and stderr for commands whose output is +needed by the caller, such as compiler identification. After the child stops, +the capture's backing bytes transfer to the caller without a second copy. A +failed output command returns both those bytes and an error containing the +diagnostic text. The capture remains in memory for the duration of the child; +an 8 MiB forwarded capture currently allocates about 8.39 MB per operation, +with the returned 8 MiB slice owned by its caller. Failed output also formats +diagnostics into the error, so the returned bytes and diagnostic string can +coexist. Disk-backed diagnostics could reduce large failure peaks, but add file +lifecycle and cleanup work and remain a follow-up option. + +Dependency preparation uses a separate command environment so proxy, private +module, checksum, and cache settings remain available. Its completed capture +uses the same ownership transfer when formatting a failure. + ## Configuration discovery The owner asks the installed `go` command for `GOHOSTOS` and `GOHOSTARCH`, then diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md index a237c88..8205c90 100644 --- a/docs/roadmap/implementation.md +++ b/docs/roadmap/implementation.md @@ -1,6 +1,6 @@ # Computational Efficiency Implementation Plan -**Status:** Stages 1–8 complete; Stages 9–11 pending. +**Status:** Stages 1–9 complete; Stages 10–11 pending. ## Purpose And Baseline @@ -498,6 +498,24 @@ forwarded bytes and meaningful error content/cause. Run Go tool, validation, build, preparation, and affected CLI tests. Re-run output benchmarks and update [Go execution mechanics](../internal/gotool.md). +**Completion:** Complete. `Tool.Run` now returns status only, while `Tool.Output` +returns combined stdout/stderr with ownership of the completed capture buffer; +the shared private execution path preserves settings and arguments. Status +success drops its capture, and both status and output failures retain complete +diagnostics and underlying process or output-writer causes. Validation and +artifact compilation use `Run`; compiler identification uses `Output`. +Preparation retains its network-capable environment and formats diagnostics +from the transferred buffer. Go tool fixtures cover forwarded output, large +failure output, cancellation, and writer failure. Go tool, validation, build, +facade, preparation, and CLI tests passed. The combined-output benchmark now +measures 1.0–1.1 µs and 4,176 B/2 allocations for 4 KiB, and 1.51–1.56 ms and +8,388,699–8,388,703 B/2 allocations for 8 MiB. The Stage 2 8 MiB result was +2.02–2.36 ms and about 16.78 MB/3 allocations, including its capture copy. +The returned 8 MiB slice remains caller-owned in memory; failed Output also +creates diagnostic text. Disk-backed diagnostics remain a measured follow-up +option because avoiding their additional lifecycle and cleanup work has not +been justified here. + ## Stage 10: Cache The Runtime Metadata Fallback **Owners:** [runtimeinfo/version.go](../../internal/runtimeinfo/version.go) and diff --git a/internal/build/build.go b/internal/build/build.go index 419e936..9b36488 100644 --- a/internal/build/build.go +++ b/internal/build/build.go @@ -118,7 +118,7 @@ func Build(ctx context.Context, request Request) (Result, error) { if err != nil { return result, fmt.Errorf("prepare Go toolchain: %w", err) } - compiler, err := goTool.Run(ctx, repositoryDir, gotool.Native, gotool.Target{}, nil, "version") + compiler, err := goTool.Output(ctx, repositoryDir, gotool.Native, gotool.Target{}, nil, "version") if err != nil { return result, fmt.Errorf("identify Go compiler: %w", err) } @@ -214,7 +214,7 @@ func gitCommit(ctx context.Context, repositoryDir string) (string, error) { func buildTarget(ctx context.Context, goTool *gotool.Tool, repositoryDir, mainPackage, outputPath, version string, target Target) error { ldflags := "-s -w -X " + versioncontract.InjectedVersionLinkerSymbol + "=" + version - _, err := goTool.Run(ctx, repositoryDir, gotool.Static, gotool.Target{OS: target.OS, Arch: target.Arch}, nil, + err := goTool.Run(ctx, repositoryDir, gotool.Static, gotool.Target{OS: target.OS, Arch: target.Arch}, nil, "build", "-o", outputPath, "-mod=readonly", "-trimpath", "-buildvcs=false", "-ldflags", ldflags, mainPackage) return err } diff --git a/internal/gotool/gotool.go b/internal/gotool/gotool.go index 477a45f..a9073e9 100644 --- a/internal/gotool/gotool.go +++ b/internal/gotool/gotool.go @@ -105,7 +105,7 @@ func PrepareBuildDependencies(ctx context.Context, directory string) error { Stderr: captured, }) if err != nil { - if output := strings.TrimSpace(string(captured.Bytes())); output != "" { + if output := strings.TrimSpace(string(captured.take())); output != "" { return fmt.Errorf("go mod download: %w: %s", err, output) } return fmt.Errorf("go mod download: %w", err) @@ -121,8 +121,19 @@ func (tool *Tool) HostTarget() Target { // Run executes a Go command with ReleaseKit's shared offline baseline. Native // commands use the host target and effective cgo configuration. Static commands // use target and disable cgo. Output, when supplied, receives stdout and stderr -// as the command runs; the returned bytes retain both streams for diagnostics. -func (tool *Tool) Run(ctx context.Context, directory string, mode Mode, target Target, output io.Writer, args ...string) ([]byte, error) { +// as the command runs. +func (tool *Tool) Run(ctx context.Context, directory string, mode Mode, target Target, output io.Writer, args ...string) error { + _, err := tool.execute(ctx, directory, mode, target, output, false, args...) + return err +} + +// Output executes a Go command and returns its combined stdout and stderr. +// Output, when supplied, also receives the bytes as the command runs. +func (tool *Tool) Output(ctx context.Context, directory string, mode Mode, target Target, output io.Writer, args ...string) ([]byte, error) { + return tool.execute(ctx, directory, mode, target, output, true, args...) +} + +func (tool *Tool) execute(ctx context.Context, directory string, mode Mode, target Target, output io.Writer, keepOutput bool, args ...string) ([]byte, error) { if len(args) == 0 { return nil, errors.New("Go command requires arguments") } @@ -136,14 +147,24 @@ func (tool *Tool) Run(ctx context.Context, directory string, mode Mode, target T Stdout: captured, Stderr: captured, }) - contents := captured.Bytes() + if forwardErr := captured.forwardingError(); forwardErr != nil { + err = errors.Join(err, fmt.Errorf("forward Go output: %w", forwardErr)) + } + contents := captured.take() if err != nil { if len(contents) != 0 { - return contents, fmt.Errorf("go %s: %w: %s", strings.Join(args, " "), err, strings.TrimSpace(string(contents))) + return outputBytes(contents, keepOutput), fmt.Errorf("go %s: %w: %s", strings.Join(args, " "), err, strings.TrimSpace(string(contents))) } - return contents, fmt.Errorf("go %s: %w", strings.Join(args, " "), err) + return outputBytes(contents, keepOutput), fmt.Errorf("go %s: %w", strings.Join(args, " "), err) } - return contents, nil + return outputBytes(contents, keepOutput), nil +} + +func outputBytes(contents []byte, keep bool) []byte { + if keep { + return contents + } + return nil } func (tool *Tool) environment(mode Mode, target Target) map[string]string { @@ -206,9 +227,10 @@ func discover(ctx context.Context, directory string, environment map[string]stri } type combinedWriter struct { - mu sync.Mutex - output io.Writer - buffer bytes.Buffer + mu sync.Mutex + output io.Writer + buffer bytes.Buffer + forwardErr error } func (writer *combinedWriter) Write(contents []byte) (int, error) { @@ -217,11 +239,29 @@ func (writer *combinedWriter) Write(contents []byte) (int, error) { if writer.output == nil { return writer.buffer.Write(contents) } - return io.MultiWriter(&writer.buffer, writer.output).Write(contents) + if _, err := writer.buffer.Write(contents); err != nil { + return 0, err + } + written, err := writer.output.Write(contents) + if err == nil && written != len(contents) { + err = io.ErrShortWrite + } + if err != nil && writer.forwardErr == nil { + writer.forwardErr = err + } + return written, err } -func (writer *combinedWriter) Bytes() []byte { +func (writer *combinedWriter) forwardingError() error { writer.mu.Lock() defer writer.mu.Unlock() - return append([]byte(nil), writer.buffer.Bytes()...) + return writer.forwardErr +} + +// take transfers the completed capture to the caller. It must only be used +// after the child process has stopped writing. +func (writer *combinedWriter) take() []byte { + writer.mu.Lock() + defer writer.mu.Unlock() + return writer.buffer.Bytes() } diff --git a/internal/gotool/gotool_test.go b/internal/gotool/gotool_test.go index fd5a23b..8f91f53 100644 --- a/internal/gotool/gotool_test.go +++ b/internal/gotool/gotool_test.go @@ -1,6 +1,7 @@ package gotool import ( + "bytes" "context" "encoding/json" "errors" @@ -62,6 +63,14 @@ func runGoPreparationFixture() int { case "fail": fmt.Fprintln(os.Stderr, "fixture dependency failure") return 23 + case "output": + fmt.Fprintln(os.Stdout, "fixture stdout") + fmt.Fprintln(os.Stderr, "fixture stderr") + return 0 + case "large-fail": + _, _ = fmt.Fprint(os.Stdout, strings.Repeat("x", 8<<20)) + fmt.Fprintln(os.Stderr, "fixture large failure") + return 23 case "wait": for { time.Sleep(time.Second) @@ -295,7 +304,7 @@ func TestPersistentSettingsPreserveCachesAndNativeCTools(t *testing.T) { func readSettings(t *testing.T, tool *Tool, mode Mode, target Target, names ...string) map[string]string { t.Helper() args := append([]string{"env", "-json"}, names...) - output, err := tool.Run(context.Background(), t.TempDir(), mode, target, nil, args...) + output, err := tool.Output(context.Background(), t.TempDir(), mode, target, nil, args...) if err != nil { t.Fatal(err) } @@ -306,6 +315,122 @@ func readSettings(t *testing.T, tool *Tool, mode Mode, target Target, names ...s return settings } +func TestOutputReturnsAndForwardsCombinedOutput(t *testing.T) { + repository, _ := installGoPreparationFixture(t) + t.Setenv("RELEASEKIT_GO_FIXTURE_MODE", "output") + tool := fixtureTool() + var forwarded bytes.Buffer + + output, err := tool.Output(context.Background(), repository, Native, Target{}, &forwarded, "version") + if err != nil { + t.Fatalf("Output() error = %v", err) + } + if !bytes.Contains(output, []byte("fixture stdout")) || !bytes.Contains(output, []byte("fixture stderr")) { + t.Fatalf("combined output = %q", output) + } + if !bytes.Equal(forwarded.Bytes(), output) { + t.Fatalf("forwarded output differs from capture: forwarded %d bytes, captured %d", forwarded.Len(), len(output)) + } +} + +func TestRunForwardsOutputWithoutReturningCapture(t *testing.T) { + repository, _ := installGoPreparationFixture(t) + t.Setenv("RELEASEKIT_GO_FIXTURE_MODE", "output") + tool := fixtureTool() + var forwarded bytes.Buffer + + if err := tool.Run(context.Background(), repository, Native, Target{}, &forwarded, "version"); err != nil { + t.Fatalf("Run() error = %v", err) + } + if !bytes.Contains(forwarded.Bytes(), []byte("fixture stdout")) || !bytes.Contains(forwarded.Bytes(), []byte("fixture stderr")) { + t.Fatalf("forwarded output = %q", forwarded.String()) + } +} + +func TestRunPreservesFailureDiagnosticsAndCause(t *testing.T) { + repository, _ := installGoPreparationFixture(t) + t.Setenv("RELEASEKIT_GO_FIXTURE_MODE", "fail") + tool := fixtureTool() + + err := tool.Run(context.Background(), repository, Native, Target{}, nil, "version") + var exitError *exec.ExitError + if !errors.As(err, &exitError) || exitError.ExitCode() != 23 || !strings.Contains(err.Error(), "fixture dependency failure") { + t.Fatalf("Run() error = %v", err) + } +} + +func TestOutputReturnsLargeFailureDiagnosticsAndCause(t *testing.T) { + repository, _ := installGoPreparationFixture(t) + t.Setenv("RELEASEKIT_GO_FIXTURE_MODE", "large-fail") + tool := fixtureTool() + + output, err := tool.Output(context.Background(), repository, Native, Target{}, nil, "version") + var exitError *exec.ExitError + wantSize := (8 << 20) + len("fixture large failure\n") + if len(output) != wantSize { + t.Fatalf("captured output = %d bytes, want %d", len(output), wantSize) + } + if !errors.As(err, &exitError) || exitError.ExitCode() != 23 || !strings.Contains(err.Error(), "fixture large failure") { + t.Fatalf("Output() error = %v", err) + } +} + +func TestOutputReturnsCancellation(t *testing.T) { + repository, recordPath := installGoPreparationFixture(t) + t.Setenv("RELEASEKIT_GO_FIXTURE_MODE", "wait") + tool := fixtureTool() + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + done := make(chan error, 1) + go func() { + _, err := tool.Output(ctx, repository, Native, Target{}, nil, "version") + done <- err + }() + + deadline := time.Now().Add(5 * time.Second) + for { + if _, err := os.Stat(recordPath); err == nil { + break + } + if time.Now().After(deadline) { + t.Fatal("Go fixture did not start") + } + time.Sleep(10 * time.Millisecond) + } + cancel() + select { + case err := <-done: + if !errors.Is(err, context.Canceled) { + t.Fatalf("Output() error = %v; want cancellation", err) + } + case <-time.After(5 * time.Second): + t.Fatal("Go fixture did not stop after cancellation") + } +} + +func TestOutputPreservesForwardingWriterFailure(t *testing.T) { + repository, _ := installGoPreparationFixture(t) + t.Setenv("RELEASEKIT_GO_FIXTURE_MODE", "output") + tool := fixtureTool() + writerErr := errors.New("fixture output writer failed") + + output, err := tool.Output(context.Background(), repository, Native, Target{}, failingOutputWriter{err: writerErr}, "version") + if !errors.Is(err, writerErr) || !strings.Contains(err.Error(), "go version") { + t.Fatalf("Output() error = %v; want wrapped writer error and command", err) + } + if len(output) == 0 { + t.Fatal("Output() returned no captured bytes after forwarding failed") + } +} + +type failingOutputWriter struct{ err error } + +func (writer failingOutputWriter) Write([]byte) (int, error) { return 0, writer.err } + +func fixtureTool() *Tool { + return &Tool{host: Target{OS: runtime.GOOS, Arch: runtime.GOARCH}, settings: map[string]string{}} +} + func clearGoSettings(t *testing.T) { t.Helper() for _, key := range []string{ diff --git a/internal/gotool/performance_test.go b/internal/gotool/performance_test.go index ac87f37..d5af2b7 100644 --- a/internal/gotool/performance_test.go +++ b/internal/gotool/performance_test.go @@ -28,7 +28,7 @@ func BenchmarkPerformanceCombinedOutput(b *testing.B) { if written, err := writer.Write(input); err != nil || written != len(input) { b.Fatalf("write captured output = %d, err %v; want %d bytes", written, err, len(input)) } - captured := writer.Bytes() + captured := writer.take() if len(captured) != len(input) { b.Fatalf("captured output = %d bytes, want %d", len(captured), len(input)) } diff --git a/internal/validation/validation.go b/internal/validation/validation.go index 0386222..001e522 100644 --- a/internal/validation/validation.go +++ b/internal/validation/validation.go @@ -93,19 +93,19 @@ func Run(ctx context.Context, request Request) (result error) { args = append(args, "-vet=off") } args = append(args, "./...") - _, err = tool.Run(ctx, request.RepositoryDir, gotool.Native, gotool.Target{}, request.Output, args...) + err = tool.Run(ctx, request.RepositoryDir, gotool.Native, gotool.Target{}, request.Output, args...) if err != nil { return &Failure{Check: GoTest, Cause: err} } if request.Settings.FullVet { - _, err = tool.Run(ctx, request.RepositoryDir, gotool.Native, gotool.Target{}, request.Output, "vet", "-mod=readonly", "./...") + err = tool.Run(ctx, request.RepositoryDir, gotool.Native, gotool.Target{}, request.Output, "vet", "-mod=readonly", "./...") if err != nil { return &Failure{Check: GoVet, Cause: err} } } for _, target := range request.Settings.CompileTargets { selected := gotool.Target{OS: target.OS, Arch: target.Arch} - _, err = tool.Run(ctx, request.RepositoryDir, gotool.Static, selected, request.Output, + err = tool.Run(ctx, request.RepositoryDir, gotool.Static, selected, request.Output, "build", "-mod=readonly", "-o", os.DevNull, "./...") if err != nil { return &Failure{