From 9cb48a70b7bb27d445a6cba627c70c29f3b696a4 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Mon, 2 Nov 2026 03:31:59 +0000 Subject: [PATCH] Bound retained Go command diagnostics --- docs/internal/gotool.md | 18 ++--- docs/roadmap/implementation.md | 2 +- internal/gotool/gotool.go | 102 +++++++++++++++++++++++++--- internal/gotool/gotool_test.go | 92 +++++++++++++++++++++++++ internal/gotool/performance_test.go | 61 ++++++++++------- 5 files changed, 230 insertions(+), 45 deletions(-) diff --git a/docs/internal/gotool.md b/docs/internal/gotool.md index 5bd0280..cde61af 100644 --- a/docs/internal/gotool.md +++ b/docs/internal/gotool.md @@ -11,18 +11,20 @@ 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. +stderr to the optional writer while the child runs, retaining only the last +1,048,576 bytes for failure diagnostics. A truncation notice identifies omitted +earlier bytes. Successful calls release their capture; failures preserve the +underlying process or forwarding error. Dependency preparation uses the same +bounded diagnostic capture. `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; -the component benchmark compares one 8 MiB write with the same output in -32 KiB writes, forwarding to `io.Discard`. Smaller writes incur buffer-growth -allocations. The [output benchmark](../../internal/gotool/performance_test.go) +the component benchmark compares full and bounded capture for modest output, +one 8 MiB write, and 8 MiB in 32 KiB writes, forwarding to `io.Discard`. +The [output benchmark](../../internal/gotool/performance_test.go) measures total allocated bytes per operation, excluding subprocess overhead and diagnostic strings. These totals count all allocations, including superseded buffers. The returned slice retains the final buffer holding the complete output. @@ -35,8 +37,8 @@ lifecycle and cleanup work and remain a follow-up option. Dependency preparation uses a separate command environment so proxy, private module, checksum, cache, and other effective settings remain available. Its `go mod download -modfile=` command overrides an alternate module file selected -through `GOFLAGS`, preserving ordinary module discovery. Its completed capture -uses the same ownership transfer when formatting a failure. +through `GOFLAGS`, preserving ordinary module discovery. Its bounded capture +is formatted into the error on failure. ## Configuration discovery diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md index 3bfe46f..dd204c7 100644 --- a/docs/roadmap/implementation.md +++ b/docs/roadmap/implementation.md @@ -49,7 +49,7 @@ the agent policy; an intermediate compilation dependency is recorded accurately. | 11 | A07: build and delivery outcomes | Complete — managed and native cleanup tests cover success, failure, cancellation, hosted completion, and CLI reporting. | | 12 | A08 | Complete — redirect boundary, origin authorization, cycle, and delivery outcome tests passed. | | 13 | A09 | Complete — bounded reads, size and digest mismatches, stream failures, cancellation, reconciliation, and public prebuilt delivery checks passed. | -| 14 | A10 | Pending | +| 14 | A10 | Complete — bounded and full capture, forwarding, error identity, concurrency, and benchmark modes passed. | | 15 | A13 | Pending | | 16 | A15 | Pending | | 17 | Remaining A19, A20 | Pending | diff --git a/internal/gotool/gotool.go b/internal/gotool/gotool.go index 8d91a91..910e624 100644 --- a/internal/gotool/gotool.go +++ b/internal/gotool/gotool.go @@ -95,7 +95,7 @@ func New(ctx context.Context, directory string) (*Tool, error) { // local Go toolchain. It preserves caller module-network settings and caches, // while disabling workspace discovery and automatic toolchain downloads. func PrepareBuildDependencies(ctx context.Context, directory string) error { - captured := &combinedWriter{} + captured := &combinedWriter{mode: captureStatus} err := process.Run(ctx, process.Invocation{ Args: []string{"go", "mod", "download", "-modfile="}, Dir: directory, @@ -107,7 +107,8 @@ func PrepareBuildDependencies(ctx context.Context, directory string) error { Stderr: captured, }) if err != nil { - if output := strings.TrimSpace(string(captured.take())); output != "" { + omitted := captured.omitted() + if output := diagnosticText(captured.take(), omitted); output != "" { return fmt.Errorf("go mod download: %w: %s", err, output) } return fmt.Errorf("go mod download: %w", err) @@ -140,7 +141,11 @@ func (tool *Tool) execute(ctx context.Context, directory string, mode Mode, targ return nil, errors.New("Go command requires arguments") } environment := tool.environment(mode, target) - captured := &combinedWriter{output: output} + modeCapture := captureStatus + if keepOutput { + modeCapture = captureFull + } + captured := &combinedWriter{output: output, mode: modeCapture} commandArgs := append([]string{"go"}, args...) err := process.Run(ctx, process.Invocation{ Args: commandArgs, @@ -152,16 +157,28 @@ func (tool *Tool) execute(ctx context.Context, directory string, mode Mode, targ if forwardErr := captured.forwardingError(); forwardErr != nil { err = errors.Join(err, fmt.Errorf("forward Go output: %w", forwardErr)) } + if err == nil && !keepOutput { + return nil, nil + } + omitted := captured.omitted() contents := captured.take() if err != nil { - if len(contents) != 0 { - return outputBytes(contents, keepOutput), fmt.Errorf("go %s: %w: %s", strings.Join(args, " "), err, strings.TrimSpace(string(contents))) + if diagnostic := diagnosticText(contents, omitted); diagnostic != "" { + return outputBytes(contents, keepOutput), fmt.Errorf("go %s: %w: %s", strings.Join(args, " "), err, diagnostic) } return outputBytes(contents, keepOutput), fmt.Errorf("go %s: %w", strings.Join(args, " "), err) } return outputBytes(contents, keepOutput), nil } +func diagnosticText(contents []byte, omitted bool) string { + text := strings.TrimSpace(string(contents)) + if omitted { + return "[earlier Go output omitted]\n" + text + } + return text +} + func outputBytes(contents []byte, keep bool) []byte { if keep { return contents @@ -232,17 +249,63 @@ type combinedWriter struct { mu sync.Mutex output io.Writer buffer bytes.Buffer + mode captureMode + tail []byte + head int + truncated bool forwardErr error } +type captureMode uint8 + +const ( + captureFull captureMode = iota + captureStatus + statusDiagnosticLimit = 1 << 20 +) + +func (writer *combinedWriter) capture(contents []byte) { + if writer.mode == captureFull { + _, _ = writer.buffer.Write(contents) + return + } + if len(contents) >= statusDiagnosticLimit { + writer.truncated = writer.truncated || len(writer.tail) != 0 || len(contents) > statusDiagnosticLimit + if cap(writer.tail) < statusDiagnosticLimit { + writer.tail = make([]byte, statusDiagnosticLimit) + } else { + writer.tail = writer.tail[:statusDiagnosticLimit] + } + copy(writer.tail, contents[len(contents)-statusDiagnosticLimit:]) + writer.head = 0 + return + } + if available := statusDiagnosticLimit - len(writer.tail); available > 0 { + n := min(available, len(contents)) + if needed := len(writer.tail) + n; needed > cap(writer.tail) { + capacity := min(statusDiagnosticLimit, max(needed, 2*cap(writer.tail))) + grown := make([]byte, len(writer.tail), capacity) + copy(grown, writer.tail) + writer.tail = grown + } + writer.tail = append(writer.tail, contents[:n]...) + contents = contents[n:] + } + if len(contents) == 0 { + return + } + writer.truncated = true + n := copy(writer.tail[writer.head:], contents) + copy(writer.tail, contents[n:]) + writer.head = (writer.head + len(contents)) % statusDiagnosticLimit +} + func (writer *combinedWriter) Write(contents []byte) (int, error) { writer.mu.Lock() defer writer.mu.Unlock() + writer.capture(contents) if writer.output == nil { - return writer.buffer.Write(contents) - } - if _, err := writer.buffer.Write(contents); err != nil { - return 0, err + return len(contents), nil } written, err := writer.output.Write(contents) if err == nil && written != len(contents) { @@ -254,6 +317,12 @@ func (writer *combinedWriter) Write(contents []byte) (int, error) { return written, err } +func (writer *combinedWriter) omitted() bool { + writer.mu.Lock() + defer writer.mu.Unlock() + return writer.truncated +} + func (writer *combinedWriter) forwardingError() error { writer.mu.Lock() defer writer.mu.Unlock() @@ -265,5 +334,18 @@ func (writer *combinedWriter) forwardingError() error { func (writer *combinedWriter) take() []byte { writer.mu.Lock() defer writer.mu.Unlock() - return writer.buffer.Bytes() + if writer.mode == captureFull { + contents := writer.buffer.Bytes() + writer.buffer = bytes.Buffer{} + return contents + } + contents := writer.tail + if writer.head != 0 { + contents = make([]byte, 0, len(writer.tail)) + contents = append(contents, writer.tail[writer.head:]...) + contents = append(contents, writer.tail[:writer.head]...) + } + writer.tail = nil + writer.head = 0 + return contents } diff --git a/internal/gotool/gotool_test.go b/internal/gotool/gotool_test.go index 69a1fcd..21de9bb 100644 --- a/internal/gotool/gotool_test.go +++ b/internal/gotool/gotool_test.go @@ -6,11 +6,13 @@ import ( "encoding/json" "errors" "fmt" + "io" "os" "os/exec" "path/filepath" "runtime" "strings" + "sync" "testing" "time" ) @@ -453,6 +455,96 @@ func TestOutputReturnsLargeFailureDiagnosticsAndCause(t *testing.T) { } } +func TestStatusCaptureRetainsTailAndForwardsAll(t *testing.T) { + for _, test := range []struct { + name string + size int + chunkSize int + }{ + {"below", statusDiagnosticLimit - 1, 8191}, + {"exact", statusDiagnosticLimit, 8191}, + {"above", statusDiagnosticLimit + 1, 8191}, + {"single oversized", statusDiagnosticLimit*2 + 3, statusDiagnosticLimit*2 + 3}, + {"wrapped small writes", statusDiagnosticLimit*3 + 17, 113}, + } { + t.Run(test.name, func(t *testing.T) { + input := make([]byte, test.size) + for i := range input { + input[i] = byte(i % 251) + } + var forwarded bytes.Buffer + writer := &combinedWriter{mode: captureStatus, output: &forwarded} + for start := 0; start < len(input); start += test.chunkSize { + chunk := input[start:min(start+test.chunkSize, len(input))] + if n, err := writer.Write(chunk); n != len(chunk) || err != nil { + t.Fatalf("write = %d, %v", n, err) + } + } + if writer.omitted() != (len(input) > statusDiagnosticLimit) || cap(writer.tail) > statusDiagnosticLimit { + t.Fatalf("omitted = %t, capacity = %d", writer.omitted(), cap(writer.tail)) + } + want := input[max(0, len(input)-statusDiagnosticLimit):] + if got := writer.take(); !bytes.Equal(got, want) || !bytes.Equal(forwarded.Bytes(), input) { + t.Fatalf("capture size = %d, forwarded size = %d", len(got), forwarded.Len()) + } + }) + } +} + +func TestStatusCaptureSerializesConcurrentWriters(t *testing.T) { + var forwarded bytes.Buffer + writer := &combinedWriter{mode: captureStatus, output: &forwarded} + var group sync.WaitGroup + for _, value := range []byte{'a', 'b'} { + group.Add(1) + go func() { + defer group.Done() + chunk := bytes.Repeat([]byte{value}, 4096) + for i := 0; i < 300; i++ { + _, _ = writer.Write(chunk) + } + }() + } + group.Wait() + contents := forwarded.Bytes() + if !writer.omitted() || !bytes.Equal(writer.take(), contents[len(contents)-statusDiagnosticLimit:]) { + t.Fatal("concurrent capture differs from forwarded tail") + } +} + +func TestStatusCommandsRetainBoundedFailureDiagnostics(t *testing.T) { + repository, _ := installGoPreparationFixture(t) + t.Setenv("RELEASEKIT_GO_FIXTURE_MODE", "large-fail") + var forwarded bytes.Buffer + err := fixtureTool().Run(context.Background(), repository, Native, Target{}, &forwarded, "version") + var exitError *exec.ExitError + if !errors.As(err, &exitError) || exitError.ExitCode() != 23 || !strings.Contains(err.Error(), "earlier Go output omitted") || !strings.Contains(err.Error(), "fixture large failure") || len(err.Error()) > statusDiagnosticLimit+200 || forwarded.Len() != (8<<20)+len("fixture large failure\n") { + t.Fatalf("Run() error = %v, forwarded = %d bytes", err, forwarded.Len()) + } + err = PrepareBuildDependencies(context.Background(), repository) + if !errors.As(err, &exitError) || exitError.ExitCode() != 23 || !strings.Contains(err.Error(), "earlier Go output omitted") || !strings.Contains(err.Error(), "fixture large failure") || len(err.Error()) > statusDiagnosticLimit+200 { + t.Fatalf("preparation error did not retain bounded diagnostics: %v", err) + } +} + +func TestStatusCommandPreservesForwardingError(t *testing.T) { + repository, _ := installGoPreparationFixture(t) + t.Setenv("RELEASEKIT_GO_FIXTURE_MODE", "output") + writerErr := errors.New("forwarding failed") + err := fixtureTool().Run(context.Background(), repository, Native, Target{}, failingOutputWriter{err: writerErr}, "version") + if !errors.Is(err, writerErr) { + t.Fatalf("Run() error = %v", err) + } + err = fixtureTool().Run(context.Background(), repository, Native, Target{}, shortOutputWriter{}, "version") + if !errors.Is(err, io.ErrShortWrite) { + t.Fatalf("Run() short-write error = %v", err) + } +} + +type shortOutputWriter struct{} + +func (shortOutputWriter) Write([]byte) (int, error) { return 0, nil } + func TestOutputReturnsCancellation(t *testing.T) { repository, recordPath := installGoPreparationFixture(t) t.Setenv("RELEASEKIT_GO_FIXTURE_MODE", "wait") diff --git a/internal/gotool/performance_test.go b/internal/gotool/performance_test.go index 9df7e63..fcc3a8c 100644 --- a/internal/gotool/performance_test.go +++ b/internal/gotool/performance_test.go @@ -20,36 +20,45 @@ func BenchmarkPerformanceCombinedOutput(b *testing.B) { } { input := bytes.Repeat([]byte("go tool diagnostic output\n"), (testCase.size+25)/26) input = input[:testCase.size] - b.Run(testCase.name+"/discard-forwarding", func(b *testing.B) { - // Verify capture and forwarding for this workload outside the measurement. - var forwarded bytes.Buffer - verification := &combinedWriter{output: &forwarded} - for offset := 0; offset < len(input); offset += testCase.chunkSize { - chunk := input[offset:min(offset+testCase.chunkSize, len(input))] - if written, err := verification.Write(chunk); err != nil || written != len(chunk) { - b.Fatalf("verify output write = %d, err %v; want %d bytes", written, err, len(chunk)) - } - } - if !bytes.Equal(verification.take(), input) || !bytes.Equal(forwarded.Bytes(), input) { - b.Fatal("captured and forwarded output must equal the complete input") - } - b.ReportAllocs() - b.SetBytes(int64(len(input))) - b.ResetTimer() - b.ReportMetric(float64(len(input)), "bytes/op") - for i := 0; i < b.N; i++ { - writer := &combinedWriter{output: io.Discard} + for _, mode := range []struct { + name string + mode captureMode + }{{"full", captureFull}, {"status", captureStatus}} { + b.Run(testCase.name+"/"+mode.name, func(b *testing.B) { + // Verify capture and forwarding for this workload outside the measurement. + var forwarded bytes.Buffer + verification := &combinedWriter{output: &forwarded, mode: mode.mode} for offset := 0; offset < len(input); offset += testCase.chunkSize { chunk := input[offset:min(offset+testCase.chunkSize, len(input))] - if written, err := writer.Write(chunk); err != nil || written != len(chunk) { - b.Fatalf("write captured output = %d, err %v; want %d bytes", written, err, len(chunk)) + if written, err := verification.Write(chunk); err != nil || written != len(chunk) { + b.Fatalf("verify output write = %d, err %v; want %d bytes", written, err, len(chunk)) } } - captured := writer.take() - if len(captured) != len(input) { - b.Fatalf("captured output = %d bytes, want %d", len(captured), len(input)) + want := input + if mode.mode == captureStatus && len(want) > statusDiagnosticLimit { + want = want[len(want)-statusDiagnosticLimit:] } - } - }) + if !bytes.Equal(verification.take(), want) || !bytes.Equal(forwarded.Bytes(), input) { + b.Fatal("captured output or forwarding did not match the selected mode") + } + b.ReportAllocs() + b.SetBytes(int64(len(input))) + b.ResetTimer() + b.ReportMetric(float64(len(input)), "bytes/op") + for i := 0; i < b.N; i++ { + writer := &combinedWriter{output: io.Discard, mode: mode.mode} + for offset := 0; offset < len(input); offset += testCase.chunkSize { + chunk := input[offset:min(offset+testCase.chunkSize, len(input))] + if written, err := writer.Write(chunk); err != nil || written != len(chunk) { + b.Fatalf("write captured output = %d, err %v; want %d bytes", written, err, len(chunk)) + } + } + captured := writer.take() + if len(captured) != len(want) { + b.Fatalf("captured output = %d bytes, want %d", len(captured), len(want)) + } + } + }) + } } }