Skip to content

Commit 1757e87

Browse files
chlowellchlowell
committed
Go: include CLI subprocess stderr output in error messages
1 parent 11dde6e commit 1757e87

File tree

2 files changed

+195
-15
lines changed

2 files changed

+195
-15
lines changed

‎go/client.go‎

Lines changed: 100 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ import (
3434
"encoding/json"
3535
"errors"
3636
"fmt"
37+
"io"
3738
"net"
3839
"os"
3940
"os/exec"
@@ -93,6 +94,9 @@ type Client struct {
9394
osProcess atomic.Pointer[os.Process]
9495
negotiatedProtocolVersion int
9596
onListModels func(ctx context.Context) ([]ModelInfo, error)
97+
stderrBuf []byte
98+
stderrBufMux sync.Mutex
99+
stderrDone chan struct{} // closed when the current process's stderr drain goroutine finishes
96100

97101
// RPC provides typed server-scoped RPC methods.
98102
// This field is nil until the client is connected via Start().
@@ -280,15 +284,17 @@ func (c *Client) Start(ctx context.Context) error {
280284
// Connect to the server
281285
if err := c.connectToServer(ctx); err != nil {
282286
killErr := c.killProcess()
287+
stderrErr := c.stderrError()
283288
c.state = StateError
284-
return errors.Join(err, killErr)
289+
return errors.Join(err, killErr, stderrErr)
285290
}
286291

287292
// Verify protocol version compatibility
288293
if err := c.verifyProtocolVersion(ctx); err != nil {
289294
killErr := c.killProcess()
295+
stderrErr := c.stderrError()
290296
c.state = StateError
291-
return errors.Join(err, killErr)
297+
return errors.Join(err, killErr, stderrErr)
292298
}
293299

294300
c.state = StateConnected
@@ -344,6 +350,9 @@ func (c *Client) Stop() error {
344350
}
345351
}
346352
c.process = nil
353+
c.stderrBufMux.Lock()
354+
c.stderrBuf = nil
355+
c.stderrBufMux.Unlock()
347356

348357
// Close external TCP connection if exists
349358
if c.isExternalServer && c.conn != nil {
@@ -417,6 +426,9 @@ func (c *Client) ForceStop() {
417426
_ = c.killProcess() // Ignore errors since we're force stopping
418427
}
419428
c.process = nil
429+
c.stderrBufMux.Lock()
430+
c.stderrBuf = nil
431+
c.stderrBufMux.Unlock()
420432

421433
// Close external TCP connection if exists
422434
if c.isExternalServer && c.conn != nil {
@@ -443,6 +455,42 @@ func (c *Client) ForceStop() {
443455
c.RPC = nil
444456
}
445457

458+
func (c *Client) getStderrOutput() string {
459+
c.stderrBufMux.Lock()
460+
defer c.stderrBufMux.Unlock()
461+
return string(c.stderrBuf)
462+
}
463+
464+
func (c *Client) stderrError() error {
465+
if output := c.getStderrOutput(); output != "" {
466+
return errors.New("stderr: " + output)
467+
}
468+
return nil
469+
}
470+
471+
func (c *Client) startStderrDrain(stderr io.ReadCloser, done chan struct{}) {
472+
go func() {
473+
defer close(done)
474+
buf := make([]byte, 1024)
475+
for {
476+
n, err := stderr.Read(buf)
477+
if n > 0 {
478+
c.stderrBufMux.Lock()
479+
// Append to buffer, keep tail if > 64KB
480+
c.stderrBuf = append(c.stderrBuf, buf[:n]...)
481+
if len(c.stderrBuf) > 64*1024 {
482+
n := copy(c.stderrBuf, c.stderrBuf[len(c.stderrBuf)-64*1024:])
483+
c.stderrBuf = c.stderrBuf[:n]
484+
}
485+
c.stderrBufMux.Unlock()
486+
}
487+
if err != nil {
488+
return
489+
}
490+
}
491+
}()
492+
}
493+
446494
func (c *Client) ensureConnected() error {
447495
if c.client != nil {
448496
return nil
@@ -1176,6 +1224,11 @@ func (c *Client) startCLIServer(ctx context.Context) error {
11761224
c.process.Env = append(c.process.Env, "COPILOT_SDK_AUTH_TOKEN="+c.options.GitHubToken)
11771225
}
11781226

1227+
// Clear previous stderr buffer
1228+
c.stderrBufMux.Lock()
1229+
c.stderrBuf = nil
1230+
c.stderrBufMux.Unlock()
1231+
11791232
if c.useStdio {
11801233
// For stdio mode, we need stdin/stdout pipes
11811234
stdin, err := c.process.StdinPipe()
@@ -1188,11 +1241,17 @@ func (c *Client) startCLIServer(ctx context.Context) error {
11881241
return fmt.Errorf("failed to create stdout pipe: %w", err)
11891242
}
11901243

1244+
stderr, err := c.process.StderrPipe()
1245+
if err != nil {
1246+
return fmt.Errorf("failed to create stderr pipe: %w", err)
1247+
}
1248+
11911249
if err := c.process.Start(); err != nil {
1192-
return fmt.Errorf("failed to start CLI server: %w", err)
1250+
closeErr := stderr.Close()
1251+
return errors.Join(fmt.Errorf("failed to start CLI server: %w", err), closeErr)
11931252
}
11941253

1195-
c.monitorProcess()
1254+
c.monitorProcess(stderr)
11961255

11971256
// Create JSON-RPC client immediately
11981257
c.client = jsonrpc2.NewClient(stdin, stdout)
@@ -1209,11 +1268,17 @@ func (c *Client) startCLIServer(ctx context.Context) error {
12091268
return fmt.Errorf("failed to create stdout pipe: %w", err)
12101269
}
12111270

1271+
stderr, err := c.process.StderrPipe()
1272+
if err != nil {
1273+
return fmt.Errorf("failed to create stderr pipe: %w", err)
1274+
}
1275+
12121276
if err := c.process.Start(); err != nil {
1213-
return fmt.Errorf("failed to start CLI server: %w", err)
1277+
closeErr := stderr.Close()
1278+
return errors.Join(fmt.Errorf("failed to start CLI server: %w", err), closeErr)
12141279
}
12151280

1216-
c.monitorProcess()
1281+
c.monitorProcess(stderr)
12171282

12181283
scanner := bufio.NewScanner(stdout)
12191284
timeout := time.After(10 * time.Second)
@@ -1223,18 +1288,21 @@ func (c *Client) startCLIServer(ctx context.Context) error {
12231288
select {
12241289
case <-timeout:
12251290
killErr := c.killProcess()
1226-
return errors.Join(errors.New("timeout waiting for CLI server to start"), killErr)
1291+
stderrErr := c.stderrError()
1292+
return errors.Join(errors.New("timeout waiting for CLI server to start"), killErr, stderrErr)
12271293
case <-c.processDone:
12281294
killErr := c.killProcess()
1229-
return errors.Join(errors.New("CLI server process exited before reporting port"), killErr)
1295+
stderrErr := c.stderrError()
1296+
return errors.Join(errors.New("CLI server process exited before reporting port"), killErr, stderrErr)
12301297
default:
12311298
if scanner.Scan() {
12321299
line := scanner.Text()
12331300
if matches := portRegex.FindStringSubmatch(line); len(matches) > 1 {
12341301
port, err := strconv.Atoi(matches[1])
12351302
if err != nil {
12361303
killErr := c.killProcess()
1237-
return errors.Join(fmt.Errorf("failed to parse port: %w", err), killErr)
1304+
stderrErr := c.stderrError()
1305+
return errors.Join(fmt.Errorf("failed to parse port: %w", err), killErr, stderrErr)
12381306
}
12391307
c.actualPort = port
12401308
return nil
@@ -1246,21 +1314,28 @@ func (c *Client) startCLIServer(ctx context.Context) error {
12461314
}
12471315

12481316
func (c *Client) killProcess() error {
1317+
var err error
12491318
if p := c.osProcess.Swap(nil); p != nil {
1250-
if err := p.Kill(); err != nil {
1251-
return fmt.Errorf("failed to kill CLI process: %w", err)
1319+
if err = p.Kill(); err != nil {
1320+
err = fmt.Errorf("failed to kill CLI process: %w", err)
12521321
}
12531322
}
1323+
if c.stderrDone != nil {
1324+
<-c.stderrDone
1325+
}
12541326
c.process = nil
1255-
return nil
1327+
return err
12561328
}
12571329

12581330
// monitorProcess signals when the CLI process exits and captures any exit error.
12591331
// processError is intentionally a local: each process lifecycle gets its own
12601332
// error value, so goroutines from previous processes can't overwrite the
12611333
// current one. Closing the channel synchronizes with readers, guaranteeing
12621334
// they see the final processError value.
1263-
func (c *Client) monitorProcess() {
1335+
func (c *Client) monitorProcess(stderr io.ReadCloser) {
1336+
stderrDone := make(chan struct{})
1337+
c.stderrDone = stderrDone
1338+
c.startStderrDrain(stderr, stderrDone)
12641339
done := make(chan struct{})
12651340
c.processDone = done
12661341
proc := c.process
@@ -1269,10 +1344,20 @@ func (c *Client) monitorProcess() {
12691344
c.processErrorPtr = &processError
12701345
go func() {
12711346
waitErr := proc.Wait()
1347+
<-stderrDone
1348+
stderr := c.getStderrOutput()
12721349
if waitErr != nil {
1273-
processError = fmt.Errorf("CLI process exited: %w", waitErr)
1350+
if stderr != "" {
1351+
processError = fmt.Errorf("CLI process exited: %w\nstderr: %s", waitErr, stderr)
1352+
} else {
1353+
processError = fmt.Errorf("CLI process exited: %w", waitErr)
1354+
}
12741355
} else {
1275-
processError = errors.New("CLI process exited unexpectedly")
1356+
if stderr != "" {
1357+
processError = fmt.Errorf("CLI process exited unexpectedly\nstderr: %s", stderr)
1358+
} else {
1359+
processError = errors.New("CLI process exited unexpectedly")
1360+
}
12761361
}
12771362
close(done)
12781363
}()

‎go/client_test.go‎

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,9 +4,11 @@ import (
44
"context"
55
"encoding/json"
66
"os"
7+
"os/exec"
78
"path/filepath"
89
"reflect"
910
"regexp"
11+
"strings"
1012
"sync"
1113
"testing"
1214
)
@@ -648,3 +650,96 @@ func TestClient_StartStopRace(t *testing.T) {
648650
t.Fatal(err)
649651
}
650652
}
653+
654+
func TestClient_StderrCapture(t *testing.T) {
655+
buildStderrFixtureCLI := func(t *testing.T) string {
656+
t.Helper()
657+
658+
tmpDir := t.TempDir()
659+
mainPath := filepath.Join(tmpDir, "main.go")
660+
binaryName := "stderr-fixture.exe"
661+
binaryPath := filepath.Join(tmpDir, binaryName)
662+
663+
source := `package main
664+
665+
import (
666+
"os"
667+
"strconv"
668+
"strings"
669+
)
670+
671+
func main() {
672+
if sizeString := os.Getenv("TEST_STDERR_SIZE"); sizeString != "" {
673+
if size, _ := strconv.Atoi(sizeString); size > 0 {
674+
_, _ = os.Stderr.WriteString(strings.Repeat("x", size))
675+
}
676+
}
677+
if message := os.Getenv("TEST_STDERR"); message != "" {
678+
_, _ = os.Stderr.WriteString(message)
679+
}
680+
os.Exit(1)
681+
}`
682+
683+
if err := os.WriteFile(mainPath, []byte(source), 0600); err != nil {
684+
t.Fatal(err)
685+
}
686+
687+
cmd := exec.Command("go", "build", "-o", binaryPath, mainPath)
688+
if output, err := cmd.CombinedOutput(); err != nil {
689+
t.Fatal(err, "\n", string(output))
690+
}
691+
692+
return binaryPath
693+
}
694+
695+
fixturePath := buildStderrFixtureCLI(t)
696+
697+
t.Run("captures stderr on startup failure", func(t *testing.T) {
698+
client := NewClient(&ClientOptions{
699+
CLIPath: fixturePath,
700+
Env: append(os.Environ(), "TEST_STDERR=something went wrong"),
701+
})
702+
703+
err := client.Start(t.Context())
704+
if err == nil {
705+
t.Fatal("Expected error, got nil")
706+
}
707+
708+
if !strings.Contains(err.Error(), "something went wrong") {
709+
t.Errorf("Expected error to contain stderr output, got: %v", err)
710+
}
711+
})
712+
713+
t.Run("caps stderr buffer", func(t *testing.T) {
714+
client := NewClient(&ClientOptions{
715+
CLIPath: fixturePath,
716+
Env: append(os.Environ(), "TEST_STDERR_SIZE=70000"),
717+
})
718+
719+
if err := client.Start(t.Context()); err == nil {
720+
t.Fatal("Expected error, got nil")
721+
}
722+
723+
output := client.getStderrOutput()
724+
if len(output) > 64*1024+100 { // Allow some slack but it should be close to 64KB
725+
t.Errorf("Expected buffer to be capped around 64KB, got %d bytes", len(output))
726+
}
727+
})
728+
729+
t.Run("clears buffer on stop", func(t *testing.T) {
730+
client := NewClient(&ClientOptions{})
731+
client.stderrBufMux.Lock()
732+
client.stderrBuf = []byte("dirty buffer")
733+
client.stderrBufMux.Unlock()
734+
735+
if err := client.Stop(); err != nil {
736+
t.Fatal(err)
737+
}
738+
739+
client.stderrBufMux.Lock()
740+
if client.stderrBuf != nil {
741+
t.Error("Expected stderrBuf to be nil after Stop")
742+
}
743+
client.stderrBufMux.Unlock()
744+
})
745+
}

0 commit comments

Comments
 (0)