Skip to content
Merged
Changes from all commits
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
20 changes: 18 additions & 2 deletions internal/mcp/connection.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package mcp

import (
"bufio"
"bytes"
"context"
"encoding/json"
Expand Down Expand Up @@ -190,11 +191,25 @@ func NewConnection(ctx context.Context, command string, args []string, env map[s
}
}

// Capture stderr to help diagnose container failures
// Capture and stream stderr to help diagnose container issues
// The SDK's CommandTransport only uses stdin/stdout for MCP protocol,
// so we can capture stderr separately for debugging
// Use a TeeReader-style approach: write to both a buffer (for error reporting)
// and to a pipe that streams to logs in real-time
var stderrBuf bytes.Buffer
cmd.Stderr = &stderrBuf
stderrPipeReader, stderrPipeWriter := io.Pipe()
cmd.Stderr = io.MultiWriter(&stderrBuf, stderrPipeWriter)

// Stream stderr to logs in a goroutine
go func() {
defer stderrPipeReader.Close()
scanner := bufio.NewScanner(stderrPipeReader)
for scanner.Scan() {
line := scanner.Text()
logger.LogInfo("backend", "[%s stderr] %s", command, line)
logConn.Printf("[stderr] %s", line)
}
Copy link

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

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

The scanner may encounter errors (e.g., if the pipe is broken or a line is too long), but these errors are not checked or logged. After the scanning loop ends, scanner.Err() should be checked to detect and log any scanning errors. This is a common pattern seen elsewhere in the codebase (e.g., internal/cmd/root.go:439 and internal/config/validation_env.go, though the latter doesn't check scanner.Err() either).

Suggested change
}
}
if err := scanner.Err(); err != nil {
logger.LogErrorMd("backend", "Error reading MCP backend stderr: %v", err)
logConn.Printf("Error reading MCP backend stderr: %v", err)
}

Copilot uses AI. Check for mistakes.
}()

Copy link

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

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

The stderrPipeWriter is only closed in the error path (line 224) but not on the success path. When the connection succeeds and returns (line 274), the pipe writer remains open, which will cause the stderr streaming goroutine to hang indefinitely, waiting for more data. This creates a goroutine leak.

The pipe writer should be stored in the Connection struct and closed when Connection.Close() is called, or alternatively, the goroutine should monitor the context cancellation to know when to stop.

Suggested change
// Ensure the stderr streaming goroutine terminates when the context is canceled
go func() {
<-ctx.Done()
// Closing the reader will unblock the scanner and allow the goroutine to exit
_ = stderrPipeReader.Close()
}()

Copilot uses AI. Check for mistakes.
logger.LogInfo("backend", "Starting MCP backend server, command=%s, args=%v", command, sanitize.SanitizeArgs(expandedArgs))
log.Printf("Starting MCP server command: %s %v", command, sanitize.SanitizeArgs(expandedArgs))
Expand All @@ -206,6 +221,7 @@ func NewConnection(ctx context.Context, command string, args []string, env map[s
session, err := client.Connect(ctx, transport, nil)
if err != nil {
cancel()
stderrPipeWriter.Close() // Close pipe to stop the stderr streaming goroutine

// Enhanced error context for debugging
logger.LogErrorMd("backend", "MCP backend connection failed, command=%s, args=%v, error=%v", command, sanitize.SanitizeArgs(expandedArgs), err)
Expand Down
Loading