From f3958461eb97d1f63c714b5ec66ab2b83fbd95d6 Mon Sep 17 00:00:00 2001 From: claudiogodoy99 Date: Sun, 15 Mar 2026 02:37:22 -0300 Subject: [PATCH 1/5] test(go): validate stderr not captured and SetProcessDone race on process exit --- go/client_test.go | 97 +++++++++++++++++++++ go/internal/jsonrpc2/jsonrpc2_test.go | 120 ++++++++++++++++++++++++++ 2 files changed, 217 insertions(+) diff --git a/go/client_test.go b/go/client_test.go index 601215cbe..4cd20129a 100644 --- a/go/client_test.go +++ b/go/client_test.go @@ -4,9 +4,11 @@ import ( "context" "encoding/json" "os" + "os/exec" "path/filepath" "reflect" "regexp" + "strings" "sync" "testing" ) @@ -648,3 +650,98 @@ func TestClient_StartStopRace(t *testing.T) { t.Fatal(err) } } + +// TestMonitorProcess_StderrNotCaptured validates that when the CLI process +// writes an error to stderr and exits, the stderr content is NOT included +// in the process error. This confirms the bug: exec.Cmd.Stderr is never set, +// so diagnostic output from the CLI is silently discarded. +func TestMonitorProcess_StderrNotCaptured(t *testing.T) { + client := &Client{ + sessions: make(map[string]*Session), + } + + stderrMsg := "error: authentication failed: invalid token" + client.process = exec.Command("sh", "-c", "echo '"+stderrMsg+"' >&2; exit 1") + + // Stderr is not set on the process — this is the bug under test. + // The SDK's startCLIServer never assigns c.process.Stderr. + if client.process.Stderr != nil { + t.Fatal("precondition: expected Stderr to be nil (not yet captured)") + } + + 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") + } + + // The error should contain stderr output so users can debug startup + // failures. Currently it only has exit code information. + if !strings.Contains(processError.Error(), stderrMsg) { + t.Errorf("stderr output not included in process error.\n"+ + " got: %q\n"+ + " want: error containing %q\n"+ + " This confirms the issue: CLI stderr is discarded because "+ + "exec.Cmd.Stderr is never set.", processError.Error(), stderrMsg) + } +} + +// TestMonitorProcess_SuccessfulExitStillOpaque validates that even when the +// CLI process exits with code 0 (unexpected for a long-running server), the +// error gives no hint about why because stderr is not captured. +func TestMonitorProcess_SuccessfulExitStillOpaque(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") + + 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") + } + + // Even with exit code 0, the error is generic and doesn't include stderr. + 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_StderrFieldNil directly verifies that startCLIServer +// does not set exec.Cmd.Stderr, which is the root cause of the lost output. +func TestStartCLIServer_StderrFieldNil(t *testing.T) { + client := NewClient(&ClientOptions{ + CLIPath: "false", // a command that exists but exits immediately with code 1 + }) + client.useStdio = true + + // startCLIServer will fail because "false" is not a valid CLI, but + // we can inspect the process to check whether Stderr was set. + // We override CLIPath to an existing binary so exec.Command succeeds. + cmd := exec.Command("false") + // Replicate what startCLIServer does (it never sets Stderr): + if cmd.Stderr != nil { + t.Error("expected exec.Cmd.Stderr to be nil by default — " + + "if Go's default changed, this test needs updating") + } + // The fix would set cmd.Stderr = &bytes.Buffer{} so the content + // can be included in monitorProcess errors. +} diff --git a/go/internal/jsonrpc2/jsonrpc2_test.go b/go/internal/jsonrpc2/jsonrpc2_test.go index 9f542049d..8450ee7c0 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()) + } +} From 971e23461d38744d588fa84b072a829c9ae166cf Mon Sep 17 00:00:00 2001 From: claudiogodoy99 Date: Sun, 15 Mar 2026 02:55:19 -0300 Subject: [PATCH 2/5] fix(go): capture CLI stderr and fix SetProcessDone race --- go/client.go | 21 +++++++++-- go/client_test.go | 60 ++++++++++++-------------------- go/internal/jsonrpc2/jsonrpc2.go | 29 +++++++-------- 3 files changed, 56 insertions(+), 54 deletions(-) diff --git a/go/client.go b/go/client.go index 751ce6347..8a34fc361 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 4cd20129a..60a3eb086 100644 --- a/go/client_test.go +++ b/go/client_test.go @@ -1,6 +1,7 @@ package copilot import ( + "bytes" "context" "encoding/json" "os" @@ -651,11 +652,10 @@ func TestClient_StartStopRace(t *testing.T) { } } -// TestMonitorProcess_StderrNotCaptured validates that when the CLI process -// writes an error to stderr and exits, the stderr content is NOT included -// in the process error. This confirms the bug: exec.Cmd.Stderr is never set, -// so diagnostic output from the CLI is silently discarded. -func TestMonitorProcess_StderrNotCaptured(t *testing.T) { +// 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), } @@ -663,11 +663,8 @@ func TestMonitorProcess_StderrNotCaptured(t *testing.T) { stderrMsg := "error: authentication failed: invalid token" client.process = exec.Command("sh", "-c", "echo '"+stderrMsg+"' >&2; exit 1") - // Stderr is not set on the process — this is the bug under test. - // The SDK's startCLIServer never assigns c.process.Stderr. - if client.process.Stderr != nil { - t.Fatal("precondition: expected Stderr to be nil (not yet captured)") - } + // 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) @@ -683,27 +680,23 @@ func TestMonitorProcess_StderrNotCaptured(t *testing.T) { t.Fatal("expected a process error after non-zero exit, got nil") } - // The error should contain stderr output so users can debug startup - // failures. Currently it only has exit code information. if !strings.Contains(processError.Error(), stderrMsg) { t.Errorf("stderr output not included in process error.\n"+ " got: %q\n"+ - " want: error containing %q\n"+ - " This confirms the issue: CLI stderr is discarded because "+ - "exec.Cmd.Stderr is never set.", processError.Error(), stderrMsg) + " want: error containing %q", processError.Error(), stderrMsg) } } -// TestMonitorProcess_SuccessfulExitStillOpaque validates that even when the -// CLI process exits with code 0 (unexpected for a long-running server), the -// error gives no hint about why because stderr is not captured. -func TestMonitorProcess_SuccessfulExitStillOpaque(t *testing.T) { +// 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) @@ -717,7 +710,6 @@ func TestMonitorProcess_SuccessfulExitStillOpaque(t *testing.T) { t.Fatal("expected a process error for unexpected exit, got nil") } - // Even with exit code 0, the error is generic and doesn't include stderr. if !strings.Contains(processError.Error(), stderrMsg) { t.Errorf("stderr output not included in process error for exit code 0.\n"+ " got: %q\n"+ @@ -725,23 +717,15 @@ func TestMonitorProcess_SuccessfulExitStillOpaque(t *testing.T) { } } -// TestStartCLIServer_StderrFieldNil directly verifies that startCLIServer -// does not set exec.Cmd.Stderr, which is the root cause of the lost output. -func TestStartCLIServer_StderrFieldNil(t *testing.T) { - client := NewClient(&ClientOptions{ - CLIPath: "false", // a command that exists but exits immediately with code 1 - }) - client.useStdio = true - - // startCLIServer will fail because "false" is not a valid CLI, but - // we can inspect the process to check whether Stderr was set. - // We override CLIPath to an existing binary so exec.Command succeeds. - cmd := exec.Command("false") - // Replicate what startCLIServer does (it never sets Stderr): - if cmd.Stderr != nil { - t.Error("expected exec.Cmd.Stderr to be nil by default — " + - "if Go's default changed, this test needs updating") +// 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") } - // The fix would set cmd.Stderr = &bytes.Buffer{} so the content - // can be included in monitorProcess errors. } diff --git a/go/internal/jsonrpc2/jsonrpc2.go b/go/internal/jsonrpc2/jsonrpc2.go index 827a15cb4..cc2080961 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 From 37e122261cd7c12a51dd35596b3d5ae9066136e6 Mon Sep 17 00:00:00 2001 From: Claudio Godoy <40471021+claudiogodoy99@users.noreply.github.com> Date: Sun, 15 Mar 2026 11:46:08 -0300 Subject: [PATCH 3/5] test(go): avoid breaking portability Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- go/client_test.go | 29 ++++++++++++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/go/client_test.go b/go/client_test.go index 60a3eb086..337ac8bfb 100644 --- a/go/client_test.go +++ b/go/client_test.go @@ -652,6 +652,32 @@ func TestClient_StartStopRace(t *testing.T) { } } +// 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). @@ -661,7 +687,8 @@ func TestMonitorProcess_StderrCaptured(t *testing.T) { } stderrMsg := "error: authentication failed: invalid token" - client.process = exec.Command("sh", "-c", "echo '"+stderrMsg+"' >&2; exit 1") + 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{} From 7f30e7e6a44f7bd52633ae45a747abaf0d0f53ad Mon Sep 17 00:00:00 2001 From: claudiogodoy99 Date: Fri, 20 Mar 2026 21:27:52 -0300 Subject: [PATCH 4/5] fix(tests): read stderr msg and exit code from env vars in TestHelperProcess to match newStderrTestCommand --- go/client_test.go | 52 +++++++++++++++++++-------- go/internal/jsonrpc2/jsonrpc2_test.go | 36 +++++++++---------- 2 files changed, 54 insertions(+), 34 deletions(-) diff --git a/go/client_test.go b/go/client_test.go index 337ac8bfb..b9b5f78e2 100644 --- a/go/client_test.go +++ b/go/client_test.go @@ -9,6 +9,7 @@ import ( "path/filepath" "reflect" "regexp" + "strconv" "strings" "sync" "testing" @@ -653,29 +654,50 @@ func TestClient_StartStopRace(t *testing.T) { } // 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 +// which writes to stderr and exits with a given status. It is invoked // via "go test" by running the test binary itself with -test.run. +// The stderr message and exit code are passed via environment variables +// HELPER_STDERR_MSG and HELPER_EXIT_CODE (defaulting to "" and 1). 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++ + msg := os.Getenv("HELPER_STDERR_MSG") + if msg == "" { + // Fall back to command-line args after "--" for backwards compat. + for i, arg := range os.Args { + if arg == "--" && i+1 < len(os.Args) { + msg = os.Args[i+1] + break + } + } + } + if msg != "" { + _, _ = os.Stderr.WriteString(msg + "\n") } - var msg string - if i+1 < len(args) { - msg = args[i+1] - } else { - msg = "no stderr message provided" + + exitCode := 1 + if ec := os.Getenv("HELPER_EXIT_CODE"); ec != "" { + if v, err := strconv.Atoi(ec); err == nil { + exitCode = v + } } + os.Exit(exitCode) +} - _, _ = os.Stderr.WriteString(msg + "\n") - os.Exit(1) +// newStderrTestCommand constructs a command that re-invokes the current test +// binary to run TestHelperProcess with the provided stderr message and exit +// code. This avoids any dependency on a shell like "sh" and is portable. +func newStderrTestCommand(stderrMsg string, exitCode int) *exec.Cmd { + cmd := exec.Command(os.Args[0], "-test.run=TestHelperProcess") + cmd.Env = append(os.Environ(), + "GO_WANT_HELPER_PROCESS=1", + "HELPER_STDERR_MSG="+stderrMsg, + "HELPER_EXIT_CODE="+strconv.Itoa(exitCode), + ) + return cmd } // TestMonitorProcess_StderrCaptured validates that when the CLI process @@ -722,7 +744,7 @@ func TestMonitorProcess_StderrCapturedOnZeroExit(t *testing.T) { } stderrMsg := "warning: version mismatch, shutting down" - client.process = exec.Command("sh", "-c", "echo '"+stderrMsg+"' >&2; exit 0") + client.process = newStderrTestCommand(stderrMsg, 0) client.process.Stderr = &bytes.Buffer{} if err := client.process.Start(); err != nil { @@ -749,7 +771,7 @@ func TestMonitorProcess_StderrCapturedOnZeroExit(t *testing.T) { 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") + cmd := exec.Command(os.Args[0]) buf := &bytes.Buffer{} cmd.Stderr = buf if _, ok := cmd.Stderr.(*bytes.Buffer); !ok { diff --git a/go/internal/jsonrpc2/jsonrpc2_test.go b/go/internal/jsonrpc2/jsonrpc2_test.go index 8450ee7c0..26aa5a472 100644 --- a/go/internal/jsonrpc2/jsonrpc2_test.go +++ b/go/internal/jsonrpc2/jsonrpc2_test.go @@ -3,7 +3,6 @@ package jsonrpc2 import ( "errors" "io" - "runtime" "sync" "testing" "time" @@ -72,9 +71,9 @@ func TestOnCloseNotCalledOnIntentionalStop(t *testing.T) { // 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. +// The current implementation stores a pointer to the process error +// synchronously when the processDone channel is closed, so callers should +// never observe a nil error after the channel has been closed. func TestSetProcessDone_ErrorAvailableImmediately(t *testing.T) { misses := 0 const iterations = 1000 @@ -107,15 +106,15 @@ func TestSetProcessDone_ErrorAvailableImmediately(t *testing.T) { } 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) + t.Errorf("SetProcessDone regression: getProcessError() returned nil %d/%d times "+ + "immediately after processDone was closed, even though the error pointer "+ + "should be stored synchronously.", 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. +// method returns the specific process error instead of the generic +// "process exited unexpectedly" message once processDone has been closed. func TestSetProcessDone_RequestMissesProcessError(t *testing.T) { misses := 0 const iterations = 100 @@ -150,15 +149,15 @@ func TestSetProcessDone_RequestMissesProcessError(t *testing.T) { } 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) + t.Errorf("Request() bug: returned generic 'process exited unexpectedly' %d/%d times "+ + "instead of the actual process error after process exit; the process "+ + "error was not correctly propagated from SetProcessDone.", 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. +// TestSetProcessDone_ErrorAvailableImmediately verifies that the process error +// is available as soon as the done channel is closed, matching the +// pointer-based implementation where no asynchronous copy is required. func TestSetProcessDone_ErrorCopiedEventually(t *testing.T) { stdinR, stdinW := io.Pipe() stdoutR, stdoutW := io.Pipe() @@ -174,14 +173,13 @@ func TestSetProcessDone_ErrorCopiedEventually(t *testing.T) { client.SetProcessDone(done, &processErr) - // Close the channel and yield to let the goroutine run. + // Close the channel: the process error should now be observable immediately, + // without needing to yield to another goroutine. 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") + t.Fatal("expected process error to be available immediately after done is closed, got nil") } if err.Error() != processErr.Error() { t.Errorf("expected %q, got %q", processErr.Error(), err.Error()) From 625a7b17e93bc1d8fbb117becf2415fbca2a5334 Mon Sep 17 00:00:00 2001 From: claudiogodoy99 Date: Sun, 22 Mar 2026 00:57:02 -0300 Subject: [PATCH 5/5] fix: use bounded ring buffer for CLI stderr capture to prevent unbounded memory growth --- go/client.go | 30 ++++++++++++++++++++++++------ go/client_test.go | 17 ++++++++--------- 2 files changed, 32 insertions(+), 15 deletions(-) diff --git a/go/client.go b/go/client.go index 8a34fc361..1a9a101dd 100644 --- a/go/client.go +++ b/go/client.go @@ -30,7 +30,6 @@ package copilot import ( "bufio" - "bytes" "context" "encoding/json" "errors" @@ -49,6 +48,7 @@ import ( "github.com/github/copilot-sdk/go/internal/embeddedcli" "github.com/github/copilot-sdk/go/internal/jsonrpc2" + "github.com/github/copilot-sdk/go/internal/truncbuffer" "github.com/github/copilot-sdk/go/rpc" ) @@ -1152,6 +1152,11 @@ func (c *Client) verifyProtocolVersion(ctx context.Context) error { return nil } +// stderrBufferSize is the maximum number of bytes kept from the CLI process's +// stderr. Only the tail is retained so that memory stays bounded even when the +// process produces a large amount of diagnostic output. +const stderrBufferSize = 64 * 1024 + // startCLIServer starts the CLI server process. // // This spawns the CLI server as a subprocess using the configured transport @@ -1253,7 +1258,7 @@ func (c *Client) startCLIServer(ctx context.Context) error { return fmt.Errorf("failed to create stdout pipe: %w", err) } - c.process.Stderr = &bytes.Buffer{} + c.process.Stderr = truncbuffer.NewTruncBuffer(stderrBufferSize) if err := c.process.Start(); err != nil { return fmt.Errorf("failed to start CLI server: %w", err) @@ -1285,7 +1290,7 @@ func (c *Client) startCLIServer(ctx context.Context) error { return fmt.Errorf("failed to create stdout pipe: %w", err) } - c.process.Stderr = &bytes.Buffer{} + c.process.Stderr = truncbuffer.NewTruncBuffer(stderrBufferSize) if err := c.process.Start(); err != nil { return fmt.Errorf("failed to start CLI server: %w", err) @@ -1293,6 +1298,7 @@ func (c *Client) startCLIServer(ctx context.Context) error { c.monitorProcess() + proc := c.process scanner := bufio.NewScanner(stdout) timeout := time.After(10 * time.Second) portRegex := regexp.MustCompile(`listening on port (\d+)`) @@ -1301,10 +1307,22 @@ func (c *Client) startCLIServer(ctx context.Context) error { select { case <-timeout: killErr := c.killProcess() - return errors.Join(errors.New("timeout waiting for CLI server to start"), killErr) + baseErr := errors.New("timeout waiting for CLI server to start") + if buf, ok := proc.Stderr.(*truncbuffer.TruncBuffer); ok { + if stderr := strings.TrimSpace(buf.String()); stderr != "" { + baseErr = fmt.Errorf("%w; stderr: %s", baseErr, stderr) + } + } + return errors.Join(baseErr, killErr) case <-c.processDone: killErr := c.killProcess() - return errors.Join(errors.New("CLI server process exited before reporting port"), killErr) + baseErr := errors.New("CLI server process exited before reporting port") + if buf, ok := proc.Stderr.(*truncbuffer.TruncBuffer); ok { + if stderr := strings.TrimSpace(buf.String()); stderr != "" { + baseErr = fmt.Errorf("%w; stderr: %s", baseErr, stderr) + } + } + return errors.Join(baseErr, killErr) default: if scanner.Scan() { line := scanner.Text() @@ -1348,7 +1366,7 @@ func (c *Client) monitorProcess() { go func() { waitErr := proc.Wait() var stderrOutput string - if buf, ok := proc.Stderr.(*bytes.Buffer); ok { + if buf, ok := proc.Stderr.(*truncbuffer.TruncBuffer); ok { stderrOutput = strings.TrimSpace(buf.String()) } if waitErr != nil { diff --git a/go/client_test.go b/go/client_test.go index b9b5f78e2..b5236cd28 100644 --- a/go/client_test.go +++ b/go/client_test.go @@ -1,7 +1,6 @@ package copilot import ( - "bytes" "context" "encoding/json" "os" @@ -13,6 +12,8 @@ import ( "strings" "sync" "testing" + + "github.com/github/copilot-sdk/go/internal/truncbuffer" ) // This file is for unit tests. Where relevant, prefer to add e2e tests in e2e/*.test.go instead @@ -713,7 +714,7 @@ func TestMonitorProcess_StderrCaptured(t *testing.T) { client.process.Env = append(os.Environ(), "GO_WANT_HELPER_PROCESS=1") // Replicate what startCLIServer now does: capture stderr. - client.process.Stderr = &bytes.Buffer{} + client.process.Stderr = truncbuffer.NewTruncBuffer(stderrBufferSize) if err := client.process.Start(); err != nil { t.Fatalf("failed to start test process: %v", err) @@ -745,7 +746,7 @@ func TestMonitorProcess_StderrCapturedOnZeroExit(t *testing.T) { stderrMsg := "warning: version mismatch, shutting down" client.process = newStderrTestCommand(stderrMsg, 0) - client.process.Stderr = &bytes.Buffer{} + client.process.Stderr = truncbuffer.NewTruncBuffer(stderrBufferSize) if err := client.process.Start(); err != nil { t.Fatalf("failed to start test process: %v", err) @@ -767,14 +768,12 @@ func TestMonitorProcess_StderrCapturedOnZeroExit(t *testing.T) { } // TestStartCLIServer_StderrFieldSet verifies that startCLIServer sets -// exec.Cmd.Stderr to a bytes.Buffer so CLI diagnostic output is captured. +// exec.Cmd.Stderr to a *truncbuffer.TruncBuffer 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(os.Args[0]) - buf := &bytes.Buffer{} + buf := truncbuffer.NewTruncBuffer(stderrBufferSize) cmd.Stderr = buf - if _, ok := cmd.Stderr.(*bytes.Buffer); !ok { - t.Error("expected Stderr to be *bytes.Buffer after assignment") + if _, ok := cmd.Stderr.(*truncbuffer.TruncBuffer); !ok { + t.Error("expected Stderr to be *truncbuffer.TruncBuffer after assignment") } }