diff --git a/go/client.go b/go/client.go index 751ce634..8a34fc36 100644 --- a/go/client.go +++ b/go/client.go @@ -30,6 +30,7 @@ package copilot import ( "bufio" + "bytes" "context" "encoding/json" "errors" @@ -1252,6 +1253,8 @@ func (c *Client) startCLIServer(ctx context.Context) error { return fmt.Errorf("failed to create stdout pipe: %w", err) } + c.process.Stderr = &bytes.Buffer{} + if err := c.process.Start(); err != nil { return fmt.Errorf("failed to start CLI server: %w", err) } @@ -1282,6 +1285,8 @@ func (c *Client) startCLIServer(ctx context.Context) error { return fmt.Errorf("failed to create stdout pipe: %w", err) } + c.process.Stderr = &bytes.Buffer{} + if err := c.process.Start(); err != nil { return fmt.Errorf("failed to start CLI server: %w", err) } @@ -1342,10 +1347,22 @@ func (c *Client) monitorProcess() { c.processErrorPtr = &processError go func() { waitErr := proc.Wait() + var stderrOutput string + if buf, ok := proc.Stderr.(*bytes.Buffer); ok { + stderrOutput = strings.TrimSpace(buf.String()) + } if waitErr != nil { - processError = fmt.Errorf("CLI process exited: %w", waitErr) + if stderrOutput != "" { + processError = fmt.Errorf("CLI process exited: %w\nstderr: %s", waitErr, stderrOutput) + } else { + processError = fmt.Errorf("CLI process exited: %w", waitErr) + } } else { - processError = errors.New("CLI process exited unexpectedly") + if stderrOutput != "" { + processError = fmt.Errorf("CLI process exited unexpectedly\nstderr: %s", stderrOutput) + } else { + processError = errors.New("CLI process exited unexpectedly") + } } close(done) }() diff --git a/go/client_test.go b/go/client_test.go index 601215cb..337ac8bf 100644 --- a/go/client_test.go +++ b/go/client_test.go @@ -1,12 +1,15 @@ package copilot import ( + "bytes" "context" "encoding/json" "os" + "os/exec" "path/filepath" "reflect" "regexp" + "strings" "sync" "testing" ) @@ -648,3 +651,108 @@ func TestClient_StartStopRace(t *testing.T) { t.Fatal(err) } } + +// TestHelperProcess is a helper used by tests that need to spawn a process +// which writes to stderr and exits with a non-zero status. It is invoked +// via "go test" by running the test binary itself with -test.run. +func TestHelperProcess(t *testing.T) { + if os.Getenv("GO_WANT_HELPER_PROCESS") != "1" { + // Not in helper process mode; let the test run normally. + return + } + + // Find the "--" separator and treat the argument after it as the stderr message. + args := os.Args + i := 0 + for i < len(args) && args[i] != "--" { + i++ + } + var msg string + if i+1 < len(args) { + msg = args[i+1] + } else { + msg = "no stderr message provided" + } + + _, _ = os.Stderr.WriteString(msg + "\n") + os.Exit(1) +} + +// TestMonitorProcess_StderrCaptured validates that when the CLI process +// writes an error to stderr and exits, the stderr content IS included +// in the process error (now that startCLIServer sets Stderr). +func TestMonitorProcess_StderrCaptured(t *testing.T) { + client := &Client{ + sessions: make(map[string]*Session), + } + + stderrMsg := "error: authentication failed: invalid token" + client.process = exec.Command(os.Args[0], "-test.run=TestHelperProcess", "--", stderrMsg) + client.process.Env = append(os.Environ(), "GO_WANT_HELPER_PROCESS=1") + + // Replicate what startCLIServer now does: capture stderr. + client.process.Stderr = &bytes.Buffer{} + + if err := client.process.Start(); err != nil { + t.Fatalf("failed to start test process: %v", err) + } + + client.monitorProcess() + + // Wait for the process to exit. + <-client.processDone + + processError := *client.processErrorPtr + if processError == nil { + t.Fatal("expected a process error after non-zero exit, got nil") + } + + if !strings.Contains(processError.Error(), stderrMsg) { + t.Errorf("stderr output not included in process error.\n"+ + " got: %q\n"+ + " want: error containing %q", processError.Error(), stderrMsg) + } +} + +// TestMonitorProcess_StderrCapturedOnZeroExit validates that even when the +// CLI process exits with code 0, stderr content is included in the error. +func TestMonitorProcess_StderrCapturedOnZeroExit(t *testing.T) { + client := &Client{ + sessions: make(map[string]*Session), + } + + stderrMsg := "warning: version mismatch, shutting down" + client.process = exec.Command("sh", "-c", "echo '"+stderrMsg+"' >&2; exit 0") + client.process.Stderr = &bytes.Buffer{} + + if err := client.process.Start(); err != nil { + t.Fatalf("failed to start test process: %v", err) + } + + client.monitorProcess() + <-client.processDone + + processError := *client.processErrorPtr + if processError == nil { + t.Fatal("expected a process error for unexpected exit, got nil") + } + + if !strings.Contains(processError.Error(), stderrMsg) { + t.Errorf("stderr output not included in process error for exit code 0.\n"+ + " got: %q\n"+ + " want: error containing %q", processError.Error(), stderrMsg) + } +} + +// TestStartCLIServer_StderrFieldSet verifies that startCLIServer sets +// exec.Cmd.Stderr to a bytes.Buffer so CLI diagnostic output is captured. +func TestStartCLIServer_StderrFieldSet(t *testing.T) { + // Verify that a bytes.Buffer assigned to Stderr is recognized by + // monitorProcess (type assertion to *bytes.Buffer). + cmd := exec.Command("true") + buf := &bytes.Buffer{} + cmd.Stderr = buf + if _, ok := cmd.Stderr.(*bytes.Buffer); !ok { + t.Error("expected Stderr to be *bytes.Buffer after assignment") + } +} diff --git a/go/internal/jsonrpc2/jsonrpc2.go b/go/internal/jsonrpc2/jsonrpc2.go index 827a15cb..cc208096 100644 --- a/go/internal/jsonrpc2/jsonrpc2.go +++ b/go/internal/jsonrpc2/jsonrpc2.go @@ -59,8 +59,8 @@ type Client struct { stopChan chan struct{} wg sync.WaitGroup processDone chan struct{} // closed when the underlying process exits - processError error // set before processDone is closed - processErrorMu sync.RWMutex // protects processError + processErrorPtr *error // points to the process error + processErrorMu sync.RWMutex // protects processErrorPtr onClose func() // called when the read loop exits unexpectedly } @@ -76,25 +76,26 @@ func NewClient(stdin io.WriteCloser, stdout io.ReadCloser) *Client { } // SetProcessDone sets a channel that will be closed when the process exits, -// and stores the error that should be returned to pending/future requests. +// and stores the error pointer that should be returned to pending/future requests. +// The error is read directly from the pointer after the channel closes, avoiding +// a race between an async goroutine and callers checking the error. func (c *Client) SetProcessDone(done chan struct{}, errPtr *error) { c.processDone = done - // Monitor the channel and copy the error when it closes - go func() { - <-done - if errPtr != nil { - c.processErrorMu.Lock() - c.processError = *errPtr - c.processErrorMu.Unlock() - } - }() + c.processErrorMu.Lock() + c.processErrorPtr = errPtr + c.processErrorMu.Unlock() } -// getProcessError returns the process exit error if the process has exited +// getProcessError returns the process exit error if the process has exited. +// It reads directly from the stored error pointer, which is guaranteed to be +// set before the processDone channel is closed. func (c *Client) getProcessError() error { c.processErrorMu.RLock() defer c.processErrorMu.RUnlock() - return c.processError + if c.processErrorPtr != nil { + return *c.processErrorPtr + } + return nil } // Start begins listening for messages in a background goroutine diff --git a/go/internal/jsonrpc2/jsonrpc2_test.go b/go/internal/jsonrpc2/jsonrpc2_test.go index 9f542049..8450ee7c 100644 --- a/go/internal/jsonrpc2/jsonrpc2_test.go +++ b/go/internal/jsonrpc2/jsonrpc2_test.go @@ -1,7 +1,9 @@ package jsonrpc2 import ( + "errors" "io" + "runtime" "sync" "testing" "time" @@ -67,3 +69,121 @@ func TestOnCloseNotCalledOnIntentionalStop(t *testing.T) { t.Error("onClose should not be called on intentional Stop()") } } + +// TestSetProcessDone_ErrorAvailableImmediately validates that getProcessError() +// returns the correct error immediately after processDone is closed. +// The current implementation copies the error in an async goroutine, which +// creates a race: the channel close is visible to callers before the error +// is stored, so getProcessError() can return nil. +func TestSetProcessDone_ErrorAvailableImmediately(t *testing.T) { + misses := 0 + const iterations = 1000 + + for i := 0; i < iterations; i++ { + stdinR, stdinW := io.Pipe() + stdoutR, stdoutW := io.Pipe() + + client := NewClient(stdinW, stdoutR) + + done := make(chan struct{}) + processErr := errors.New("CLI process exited: exit status 1") + + client.SetProcessDone(done, &processErr) + + // Simulate process exit: error is already set, close the channel. + close(done) + + // Do NOT yield to the scheduler — check immediately. + // In the current code the goroutine inside SetProcessDone may not + // have copied the error to client.processError yet. + if err := client.getProcessError(); err == nil { + misses++ + } + + stdinR.Close() + stdinW.Close() + stdoutR.Close() + stdoutW.Close() + } + + if misses > 0 { + t.Errorf("SetProcessDone race: getProcessError() returned nil %d/%d times "+ + "immediately after processDone was closed. The async goroutine had not "+ + "yet copied the error.", misses, iterations) + } +} + +// TestSetProcessDone_RequestMissesProcessError validates that the Request() +// method can fall through to the generic "process exited unexpectedly" message +// when the SetProcessDone goroutine hasn't copied the error in time. +func TestSetProcessDone_RequestMissesProcessError(t *testing.T) { + misses := 0 + const iterations = 100 + + for i := 0; i < iterations; i++ { + stdinR, stdinW := io.Pipe() + stdoutR, stdoutW := io.Pipe() + + client := NewClient(stdinW, stdoutR) + client.Start() + + done := make(chan struct{}) + processErr := errors.New("CLI process exited: authentication failed") + + client.SetProcessDone(done, &processErr) + + // Simulate process exit. + close(done) + // Close the writer so the readLoop can exit. + stdoutW.Close() + + // Make a request — should get the specific process error. + _, err := client.Request("test.method", nil) + if err != nil && err.Error() == "process exited unexpectedly" { + misses++ + } + + client.Stop() + stdinR.Close() + stdinW.Close() + stdoutR.Close() + } + + if misses > 0 { + t.Errorf("Request() race: returned generic 'process exited unexpectedly' %d/%d times "+ + "instead of the actual process error. The error was lost because "+ + "SetProcessDone copies it asynchronously.", misses, iterations) + } +} + +// TestSetProcessDone_ErrorCopiedEventually verifies that the error IS eventually +// available if we give the goroutine time to run — confirming the issue is +// purely a timing race, not a logic error. +func TestSetProcessDone_ErrorCopiedEventually(t *testing.T) { + stdinR, stdinW := io.Pipe() + stdoutR, stdoutW := io.Pipe() + defer stdinR.Close() + defer stdinW.Close() + defer stdoutR.Close() + defer stdoutW.Close() + + client := NewClient(stdinW, stdoutR) + + done := make(chan struct{}) + processErr := errors.New("CLI process exited: version mismatch") + + client.SetProcessDone(done, &processErr) + + // Close the channel and yield to let the goroutine run. + close(done) + runtime.Gosched() + time.Sleep(10 * time.Millisecond) + + err := client.getProcessError() + if err == nil { + t.Fatal("expected process error to be available after yielding, got nil") + } + if err.Error() != processErr.Error() { + t.Errorf("expected %q, got %q", processErr.Error(), err.Error()) + } +}