Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 19 additions & 5 deletions mcp/cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,19 @@ import (
"time"
)

const (
defaultTerminateDuration = 5 * time.Second
minTerminateDuration = 1 * time.Second
)

// A CommandTransport is a [Transport] that runs a command and communicates
// with it over stdin/stdout, using newline-delimited JSON.
type CommandTransport struct {
Command *exec.Cmd
// TerminateDuration controls how long Close waits after closing stdin
// for the process to exit before sending SIGTERM.
// If less than 1 second (including zero or negative), the default of 5s is used.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there should be a minimum. 0 should be 5s, but 1 (nanosecond) should be allowed, in case someone doesn't want to wait.

TerminateDuration time.Duration
}

// NewCommandTransport returns a [CommandTransport] that runs the given command
Expand Down Expand Up @@ -46,15 +55,20 @@ func (t *CommandTransport) Connect(ctx context.Context) (Connection, error) {
if err := t.Command.Start(); err != nil {
return nil, err
}
return newIOConn(&pipeRWC{t.Command, stdout, stdin}), nil
terminateDuration := t.TerminateDuration
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shorter name for the local: its scope is small. td is fine.

if terminateDuration < minTerminateDuration {
terminateDuration = defaultTerminateDuration
}
return newIOConn(&pipeRWC{t.Command, stdout, stdin, terminateDuration}), nil
}

// A pipeRWC is an io.ReadWriteCloser that communicates with a subprocess over
// stdin/stdout pipes.
type pipeRWC struct {
cmd *exec.Cmd
stdout io.ReadCloser
stdin io.WriteCloser
cmd *exec.Cmd
stdout io.ReadCloser
stdin io.WriteCloser
terminateDuration time.Duration
}

func (s *pipeRWC) Read(p []byte) (n int, err error) {
Expand Down Expand Up @@ -85,7 +99,7 @@ func (s *pipeRWC) Close() error {
select {
case err := <-resChan:
return err, true
case <-time.After(5 * time.Second):
case <-time.After(s.terminateDuration):
}
return nil, false
}
Expand Down
67 changes: 67 additions & 0 deletions mcp/cmd_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,73 @@ func createServerCommand(t *testing.T, serverName string) *exec.Cmd {
return cmd
}

func TestCommandTransportTerminateDuration(t *testing.T) {
if runtime.GOOS == "windows" {
t.Skip("requires POSIX signals")
}
requireExec(t)

tests := []struct {
name string
duration time.Duration
wantMaxDuration time.Duration
}{
{
name: "default duration (zero)",
duration: 0,
wantMaxDuration: 6 * time.Second, // default 5s + buffer
},
{
name: "below minimum duration",
duration: 500 * time.Millisecond,
wantMaxDuration: 6 * time.Second, // should use default 5s + buffer
},
{
name: "custom valid duration",
duration: 2 * time.Second,
wantMaxDuration: 3 * time.Second, // custom 2s + buffer
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
ctx, cancel := context.WithCancel(context.Background())
defer cancel()

// Use a command that won't exit when stdin is closed
cmd := exec.Command("sleep", "3600")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If something goes wrong, this test could wait an hour (in theory; I believe tests time out after 10 min by default). What about 20 seconds?

transport := &mcp.CommandTransport{
Command: cmd,
TerminateDuration: tt.duration,
}

conn, err := transport.Connect(ctx)
if err != nil {
t.Fatal(err)
}

start := time.Now()
err = conn.Close()
elapsed := time.Since(start)

// Close() may return "signal: terminated" when the subprocess is killed,
// which is expected behavior for our test with a non-responsive subprocess
if err != nil && err.Error() != "signal: terminated" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the returned error an exec.ExitError, or something that wraps it? If so, you should be able to get the exit code.

t.Fatalf("Close() failed with unexpected error: %v", err)
}

if elapsed > tt.wantMaxDuration {
t.Errorf("Close() took %v, expected at most %v", elapsed, tt.wantMaxDuration)
}

// Ensure the process was actually terminated
if cmd.Process != nil {
cmd.Process.Kill()
}
})
}
}

func requireExec(t *testing.T) {
t.Helper()

Expand Down