fix: prevent double-starting stdio transport in client #564
Conversation
Avoid starting stdio transport twice by checking transport type before calling Start(). The stdio transport from NewStdioMCPClientWithOptions is already started and doesn't need to be started again.
WalkthroughClient.Start now skips calling transport.Start for stdio transports; handler wiring and other Start behavior remain. NewStdioMCPClient/NewStdioMCPClientWithOptions doc-comments were adjusted to state the stdio transport is started automatically; no exported API signatures changed. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
client/client.go (5)
81-89: *Decouple from concrete type: prefer a small capability interface over checking transport.StdioRelying on a concrete type assertion couples Client to a specific transport implementation and will miss wrappers/decorators around stdio. Introduce a tiny capability method (e.g., IsAutoStarted) and feature-detect it instead. This avoids breakage if new auto-start transports are added later or if stdio is wrapped.
Apply this localized diff to client/client.go:
- if _, ok := c.transport.(*transport.Stdio); !ok { - // the stdio transport from NewStdioMCPClientWithOptions - // is already started, dont start again. - // - // Start the transport for other transport types - err := c.transport.Start(ctx) - if err != nil { - return err - } - } + // Only start transports that are not auto-started. + if a, ok := c.transport.(interface{ IsAutoStarted() bool }); !ok || !a.IsAutoStarted() { + if err := c.transport.Start(ctx); err != nil { + return fmt.Errorf("transport start failed: %w", err) + } + }And implement the capability on stdio (transport/stdio.go):
// IsAutoStarted reports that stdio is started by its constructor helpers. func (s *Stdio) IsAutoStarted() bool { return true }If you prefer not to add a method to Stdio, an alternative is a helper in the transport package:
// transport/helpers.go func IsAutoStarted(t Interface) bool { switch t.(type) { case *Stdio: return true default: return false } }Then:
- if _, ok := c.transport.(*transport.Stdio); !ok { + if !transport.IsAutoStarted(c.transport) { ... }
76-106: Make Client.Start idempotent and concurrency-safe with sync.OnceToday, Start can be called repeatedly (or concurrently) for non-stdio transports, potentially double-starting connections/goroutines. Guard the whole Start setup with a sync.Once to make it safe and predictable across all transports.
Proposed changes:
type Client struct { transport transport.Interface + startOnce sync.Once // ... }func (c *Client) Start(ctx context.Context) error { if c.transport == nil { return fmt.Errorf("transport is nil") } - if _, ok := c.transport.(*transport.Stdio); !ok { - // the stdio transport from NewStdioMCPClientWithOptions - // is already started, dont start again. - // - // Start the transport for other transport types - err := c.transport.Start(ctx) - if err != nil { - return err - } - } - c.transport.SetNotificationHandler(func(notification mcp.JSONRPCNotification) { - c.notifyMu.RLock() - defer c.notifyMu.RUnlock() - for _, handler := range c.notifications { - handler(notification) - } - }) - if bidirectional, ok := c.transport.(transport.BidirectionalInterface); ok { - bidirectional.SetRequestHandler(c.handleIncomingRequest) - } - return nil + var startErr error + c.startOnce.Do(func() { + // Only start transports that are not auto-started. + if a, ok := c.transport.(interface{ IsAutoStarted() bool }); !ok || !a.IsAutoStarted() { + if err := c.transport.Start(ctx); err != nil { + startErr = fmt.Errorf("transport start failed: %w", err) + return + } + } + c.transport.SetNotificationHandler(func(notification mcp.JSONRPCNotification) { + c.notifyMu.RLock() + defer c.notifyMu.RUnlock() + for _, handler := range c.notifications { + handler(notification) + } + }) + if bidirectional, ok := c.transport.(transport.BidirectionalInterface); ok { + bidirectional.SetRequestHandler(c.handleIncomingRequest) + } + }) + return startErr }This keeps Start a no-op on subsequent calls while preserving handler behavior.
86-88: Wrap Start error with context for easier diagnosisWrap the error to make logs clearer about which step failed.
- err := c.transport.Start(ctx) - if err != nil { - return err - } + if err := c.transport.Start(ctx); err != nil { + return fmt.Errorf("transport start failed: %w", err) + }
81-89: Add targeted tests to guard against regressionsRecommend adding tests with fakes to assert:
- Start is not called for auto-start transports.
- Start is called exactly once for non-auto-start transports.
- Handlers are registered even when Start is skipped for stdio.
Example fake transport scaffold (for a _test.go file):
type fakeTransport struct { started int notify func(mcp.JSONRPCNotification) transport.Interface } func (f *fakeTransport) Start(context.Context) error { f.started++; return nil } func (f *fakeTransport) SetNotificationHandler(h func(mcp.JSONRPCNotification)) { f.notify = h } func (f *fakeTransport) SendRequest(ctx context.Context, r transport.JSONRPCRequest) (*transport.JSONRPCResponse, error) { return &transport.JSONRPCResponse{Result: json.RawMessage(`{}`)}, nil } func (f *fakeTransport) GetSessionId() string { return "" } type autoStartFake struct{ fakeTransport } func (a *autoStartFake) IsAutoStarted() bool { return true }Sample assertions:
t.Run("non-auto-start transport is started once", func(t *testing.T) { ft := &fakeTransport{} c := client.NewClient(ft) require.NoError(t, c.Start(context.Background())) require.Equal(t, 1, ft.started) }) t.Run("auto-start transport is not started by client", func(t *testing.T) { ft := &autoStartFake{} c := client.NewClient(ft) require.NoError(t, c.Start(context.Background())) require.Equal(t, 0, ft.started) })If you want, I can open a follow-up PR with these tests.
81-89: Set notification and request handlers before calling transport.StartTo prevent a race where a transport may begin emitting notifications immediately upon startup—before the client has registered its handlers—you should register both the notification and request handlers before starting the transport. In
client/client.go, update theStartmethod as follows:func (c *Client) Start(ctx context.Context) error { if c.transport == nil { return fmt.Errorf("transport is nil") } + // 1. Register handlers before any Start call to avoid missed events c.transport.SetNotificationHandler(func(notification mcp.JSONRPCNotification) { c.notifyMu.RLock() defer c.notifyMu.RUnlock() for _, handler := range c.notifications { handler(notification) } }) if bidirectional, ok := c.transport.(transport.BidirectionalInterface); ok { bidirectional.SetRequestHandler(c.handleIncomingRequest) } - // 2. Start the transport (skip for auto-started transports like Stdio) - if a, ok := c.transport.(interface{ IsAutoStarted() bool }); !ok || !a.IsAutoStarted() { - if err := c.transport.Start(ctx); err != nil { - return fmt.Errorf("transport start failed: %w", err) - } - } + // 2. Now start the transport if it isn’t already auto-started + if a, ok := c.transport.(interface{ IsAutoStarted() bool }); !ok || !a.IsAutoStarted() { + if err := c.transport.Start(ctx); err != nil { + return fmt.Errorf("transport start failed: %w", err) + } + } return nil }• Apply the same reordering to the analogous block at lines 92–103 in this file.
• For theStdiotransport (returned byNewStdioMCPClientWithOptions), which auto-starts internally, consider exposing an explicit initialization step or delaying itsStartcall until after handler registration—otherwise early notifications will be dropped.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
client/client.go(1 hunks)client/stdio.go(0 hunks)
💤 Files with no reviewable changes (1)
- client/stdio.go
🧰 Additional context used
🧬 Code graph analysis (1)
client/client.go (1)
client/transport/stdio.go (1)
Stdio(22-42)
🔇 Additional comments (2)
client/client.go (2)
81-90: LGTM: prevents double-starting stdio while preserving handler wiringThe conditional check correctly avoids calling Start on stdio transports created by NewStdioMCPClientWithOptions and still registers notification and request handlers. This aligns with Issue #193’s objective.
82-85: Nit: refine stdio transport comment grammar and verify constructor nameVerified that the only stdio constructors in this package are
NewStdioMCPClient(which delegates) andNewStdioMCPClientWithOptions, and the comment correctly references the latter. To improve clarity and fix minor grammar:In
client/client.goaround line 82:- // the stdio transport from NewStdioMCPClientWithOptions - // is already started, dont start again. + // The stdio transport returned by NewStdioMCPClientWithOptions + // is already started; don't start it again here.
Description
Prevents double-starting of stdio transport in client by adding a type check to only start transport for non-stdio transport types. The stdio transport from
NewStdioMCPClientWithOptionsis already started and doesn't need to be started again.Fixes #193
Type of Change
Checklist
Additional Information
The
Startmethod is only way to register notification handlers.Summary by CodeRabbit