Go: include CLI subprocess stderr output in error messages#606
Open
chlowell wants to merge 2 commits intogithub:mainfrom
Open
Go: include CLI subprocess stderr output in error messages#606chlowell wants to merge 2 commits intogithub:mainfrom
chlowell wants to merge 2 commits intogithub:mainfrom
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves error messages in the Go SDK by capturing the CLI subprocess's stderr output and appending it to errors when the subprocess exits unexpectedly. A goroutine drains stderr into a ring buffer (capped at 64 KB), and the buffer contents are appended to error messages when the process fails.
Changes:
- Adds a stderr drain goroutine (
startStderrDrain) and a 64 KB ring buffer (stderrBuf) to theClientstruct, protected bystderrBufMuxand synchronized via astderrDonechannel. - Updates
killProcess,monitorProcess, and all error paths instartCLIServerandStartto wait for the drain goroutine and include stderr in error messages. - Adds
TestClient_StderrCapturewith three subtests covering startup failure capture, buffer capping, and buffer clearing on stop.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
go/client.go |
Core implementation: stderrBuf/stderrBufMux/stderrDone fields, startStderrDrain, getStderrOutput, stderrError, updated killProcess/monitorProcess/startCLIServer/Start/Stop/ForceStop |
go/client_test.go |
New TestClient_StderrCapture test: builds a Go fixture binary that writes to stderr and exits, then verifies capture, capping, and clearing behavior |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When the CLI subprocess fails (crash, bad args, etc.), the Go SDK returns a generic error that doesn't explain why the CLI exited. With this PR, the SDK appends the subprocess's stderr to these errors:
process exited unexpectedly failed to kill CLI process: os: process already finished +stderr: something went wrongThe implementation is a little complex because of the synchronization required to prevent races and leaks, and enable a client instance to start a new subprocess while concurrently cleaning up after an old one. The high-level gist is that the client uses a ring buffer (capped at 64 KB) to track stderr and drains that into an error message when the subprocess exits.