fix: prevent double-starting stdio transport in client - #564

client/client.go (5)

81-89: *Decouple from concrete type: prefer a small capability interface over checking transport.Stdio

Relying 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.Once

Today, 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 diagnosis

Wrap 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 regressions

Recommend 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.Start

To 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 the Start method 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 the Stdio transport (returned by NewStdioMCPClientWithOptions), which auto-starts internally, consider exposing an explicit initialization step or delaying its Start call until after handler registration—otherwise early notifications will be dropped.

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.