fix: make transport Start() idempotent to resolve issue #583

[ezynda3]

Description

Fixes #583 by making Start() idempotent across all transport types. The bug caused client.Initialize() to fail with "transport error: stdio client not started" when using the direct transport creation pattern with transport.NewStdioWithOptions().

Root cause: PR #564 modified client.Start() to skip starting *transport.Stdio transports based on type checking. This worked for NewStdioMCPClientWithOptions() (which pre-starts the transport), but failed for direct transport.NewStdioWithOptions() usage.

Solution: Made Start() idempotent across all transport types instead of type-checking. Multiple Start() calls are now safe with no side effects.

Fixes #583

Type of Change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• MCP spec compatibility implementation
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation update
• Code refactoring (no functional changes)
• Performance improvement
• Tests only (no functional changes)
• Other (please describe):

Checklist

• My code follows the code style of this project
• I have performed a self-review of my own code
• I have added tests that prove my fix is effective or that my feature works
• I have updated the documentation accordingly

Changes Made

• Stdio: Added started flag; returns nil on subsequent calls
• SSE: Changed to return nil instead of error when already started
• StreamableHTTP: Added early return check using initialized channel
• InProcess: Added started flag; returns nil on subsequent calls
• Client: Removed type-checking hack; always calls transport.Start()

The started flag is reset on failure to allow retry.

Tests Added

Regression Tests (client/issue583_test.go)

• TestDirectTransportCreation: Tests exact bug scenario from bug: client.Intialize errors when using transport.NewStdioWithOptions #583
• TestNewStdioMCPClientWithOptions: Verifies backward compatibility
• TestMultipleClientStartCalls: Tests client-level idempotency

Idempotency Tests (client/transport/stdio_idempotent_test.go)

• TestStdio_StartIdempotency: Tests multiple Start() calls
• TestStdio_StartFailureReset: Tests failed Start() retry
• TestStdio_StartWithOptions_Idempotent: Tests with custom options

Transport Tests (client/transport/idempotent_all_transports_test.go)

• TestStreamableHTTP_StartIdempotency
• TestInProcessTransport_StartIdempotency
• TestInProcessTransport_StartFailureReset

Additional Information

Backward Compatibility: Fully backward compatible - no interface changes, no new public APIs. Existing code continues to work, with the added benefit that Start() can now be called multiple times safely.

Test Results: All existing tests pass (72+ tests). New tests verify the fix and prevent regression.

Usage Example:

// This pattern now works correctly (was broken before)
tport := transport.NewStdioWithOptions(command, env, args)
cli := client.NewClient(tport)

if err := cli.Start(ctx); err != nil {
    return fmt.Errorf("starting MCP server: %w", err)
}

result, err := cli.Initialize(ctx, mcp.InitializeRequest{})
// ✅ Works! No "stdio client not started" error

Summary by CodeRabbit

• Bug Fixes

  • Client and transports now handle repeated Start calls without errors, improving stability across Stdio, SSE, Streamable HTTP, and In-Process connections.
  • Startup state resets correctly after failures, allowing reliable retries and initialization.

• Tests

  • Added comprehensive tests covering idempotent Start behavior, end-to-end initialization, failure recovery, and custom command scenarios, with cross-platform support.

[coderabbitai]

Walkthrough

Makes Start idempotent across client and transports. client.Start always calls transport.Start. In-process, stdio, SSE, and StreamableHTTP transports now guard repeated Start calls. Adds tests for stdio and transport idempotency and a regression test for issue #583 validating stdio initialization via options.

Changes

Cohort / File(s)	Summary of changes
Client start behavior client/client.go	client.Start now unconditionally calls transport Start and returns any error, removing stdio-specific branching.
Transport Start idempotency client/transport/inprocess.go, client/transport/stdio.go, client/transport/sse.go, client/transport/streamable_http.go	Adds started state and mutex guards (where applicable); Start returns nil if already started; ensures started is reset on failures; StreamableHTTP short-circuits if already initialized.
Stdio transport idempotency tests client/transport/stdio_idempotent_test.go	Tests multiple Start calls, failure recovery resets, and custom command function invoked only once.
Cross-transport idempotency tests client/transport/idempotent_all_transports_test.go	Verifies Start idempotency for StreamableHTTP and InProcess transports; skips SSE test requiring real server. Checks started state post-success.
Regression tests for issue #583 client/issue583_test.go	Adds tests to create mock stdio server, start client with NewStdioWithOptions/NewStdioMCPClientWithOptions, call Initialize, and validate multiple Start calls.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• fix: prevent double-starting stdio transport in client #564 — Adjusted client Start behavior around stdio; directly related to always calling Start and issue bug: client.Intialize errors when using transport.NewStdioWithOptions #583.
• feat: client-side streamable-http transport supports continuously listening  #317 — Modified StreamableHTTP Start/initialization flow; overlaps with this PR’s idempotent Start changes.
• Add InProcessTransport  #189 — Changed InProcess transport Start semantics; related to adding started flag and idempotency here.

Suggested reviewers

• rwjblue-glean

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The title clearly and concisely summarizes the primary change—making Start() idempotent across transports—and references the issue it resolves, making it immediately understandable to reviewers.
Linked Issues Check	✅ Passed	The implementation addresses all objectives from issue #583 by removing type-based skips in client.Start, adding idempotent guards and failure resets in each transport, and restoring backward compatibility for direct NewStdioWithOptions usage.
Out of Scope Changes Check	✅ Passed	All changes are focused on making Start() idempotent and adding related tests, with no unrelated refactoring or feature additions.
Description Check	✅ Passed	The description follows the repository template by providing a concise summary with a Fixes reference, a populated Type of Change section, a completed Checklist, and detailed breakdowns of changes and tests while appropriately omitting non-applicable sections.
Docstring Coverage	✅ Passed	Docstring coverage is 91.67% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

• 📝 Generate Docstrings

🧪 Generate unit tests

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/issue-583-idempotent-start

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 61b5d9e and 6e7b23d.

📒 Files selected for processing (8)

• client/client.go (1 hunks)
• client/issue583_test.go (1 hunks)
• client/transport/idempotent_all_transports_test.go (1 hunks)
• client/transport/inprocess.go (2 hunks)
• client/transport/sse.go (1 hunks)
• client/transport/stdio.go (2 hunks)
• client/transport/stdio_idempotent_test.go (1 hunks)
• client/transport/streamable_http.go (1 hunks)

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2025-08-14T16:35:34.100Z

Learnt from: JBUinfo
PR: mark3labs/mcp-go#548
File: server/stdio.go:239-300
Timestamp: 2025-08-14T16:35:34.100Z
Learning: In server/stdio.go, the s.mu mutex protects access to the writer field, but does not protect the actual write operations to stdout. Concurrent write operations to the same stdout stream need to be synchronized using the same mutex to prevent byte interleaving and JSON corruption.

Applied to files:

• client/transport/stdio.go

🧬 Code graph analysis (4)

client/transport/inprocess.go (1)

server/inprocess_session.go (1)

• NewInProcessSessionWithHandlers (43-50)

client/issue583_test.go (4)

client/transport/stdio.go (1)

• NewStdioWithOptions (104-126)

client/client.go (1)

• NewClient (70-80)

mcp/types.go (4)

• InitializeRequest (433-437)
• Params (180-180)
• LATEST_PROTOCOL_VERSION (104-104)
• Implementation (521-524)

client/stdio.go (1)

• NewStdioMCPClientWithOptions (32-45)

client/transport/idempotent_all_transports_test.go (2)

client/transport/streamable_http.go (1)

• NewStreamableHTTP (131-161)

client/transport/inprocess.go (1)

• NewInProcessTransport (40-44)

client/transport/stdio_idempotent_test.go (2)

client/transport/stdio.go (3)

• NewStdio (91-97)
• NewStdioWithOptions (104-126)
• WithCommandFunc (59-63)

mcp/types.go (1)

• NewRequestId (262-264)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

• GitHub Check: coverage
• GitHub Check: test

🔇 Additional comments (17)

client/client.go (1)

89-93: LGTM! Simplified client startup logic.

The change correctly delegates idempotency to individual transports, removing the type-based conditional that caused issue #583. All transports now handle repeated Start() calls safely.

client/transport/streamable_http.go (1)

165-170: LGTM! Channel-based idempotency is correct.

The non-blocking select on the initialized channel correctly implements idempotent Start(). Since channels are closed at most once and closing broadcasts to all readers, this pattern is safe for concurrent access.

client/transport/stdio.go (2)

43-44: LGTM! Proper concurrency guards added.

The started flag and startedMu mutex correctly implement idempotent Start() with safe retry on failure. The implementation follows the pattern established for InProcess transport.

---

129-146: LGTM! Idempotent Start with correct failure reset.

The implementation correctly:

1. Guards against concurrent Start() calls with mutex
2. Returns early if already started (idempotent)
3. Resets the started flag if spawnCommand fails (allows retry)

This resolves issue #583 by ensuring Start() can be called safely multiple times.

client/transport/inprocess.go (2)

22-23: LGTM! Proper concurrency guards added.

The started flag and startedMu mutex correctly implement idempotent Start() following the same pattern as Stdio transport.

---

60-78: LGTM! Idempotent Start with correct failure reset.

The implementation correctly:

1. Guards against concurrent Start() calls with mutex
2. Returns early if already started (idempotent)
3. Resets the started flag if session registration fails (allows retry)

Pattern is consistent with Stdio transport implementation.

client/issue583_test.go (3)

17-67: LGTM! Regression test for issue #583.

This test correctly validates the fix for the reported bug where using transport.NewStdioWithOptions directly followed by client.Start() would fail. The test verifies that Initialize succeeds after Start, which was the core issue.

---

70-122: LGTM! Backwards compatibility test.

This test validates that the existing NewStdioMCPClientWithOptions pattern continues to work and that calling Start() again is idempotent (no error). Good coverage of the compatibility requirement.

---

125-184: LGTM! Multiple Start() calls test.

This test thoroughly validates Start() idempotency by calling it three times and verifying the client still functions correctly. The test confirms the transport remains usable after multiple Start() calls.

client/transport/stdio_idempotent_test.go (3)

15-68: LGTM! Comprehensive idempotency test.

The test validates that multiple Start() calls are safe and that the transport remains functional after repeated starts. Good use of SendRequest to verify transport health.

---

71-95: LGTM! Failure reset test.

This test correctly verifies that the started flag is reset after a failed Start(), allowing retries. Direct access to the flag via mutex is appropriate for white-box testing of the internal state.

---

98-146: LGTM! CommandFunc invocation test.

This test validates that the custom command function is only invoked once on the first Start() and not on subsequent idempotent calls. This is critical to prevent spawning multiple processes.

client/transport/idempotent_all_transports_test.go (4)

12-14: Appropriate skip for SSE test.

Skipping the SSE test is reasonable since it requires a real HTTP server. SSE idempotency should be covered in integration tests.

---

17-44: LGTM! StreamableHTTP idempotency test.

The test validates that StreamableHTTP.Start() is idempotent across three calls. The 1-second timeout is sufficient for the channel-based idempotency check.

---

47-76: LGTM! InProcess idempotency test.

The test validates that InProcessTransport.Start() is idempotent across three calls, consistent with the pattern used for other transports.

---

79-111: LGTM! InProcess started flag verification.

The test acknowledges the difficulty of simulating InProcess Start() failures and instead verifies that the started flag is set after successful Start(). This is acceptable for validating the state management logic.

client/transport/sse.go (1)

117-119: Reviewer concern about resetting started flag is incorrect
c.started.Store(true) is only called at line 189 after a successful Start(), so on failure the flag remains false and retries won’t be skipped.

Likely an incorrect or invalid review comment.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[github-actions]

Merging this branch changes the coverage (1 decrease, 1 increase)

Impacted Packages	Coverage Δ	🤖
github.com/mark3labs/mcp-go/client	67.39% (-0.12%)	👎
github.com/mark3labs/mcp-go/client/transport	67.74% (+0.72%)	👍

---

Coverage by file

Changed files (no unit tests)

Changed File	Coverage Δ	Total	Covered	Missed	🤖
github.com/mark3labs/mcp-go/client/client.go	65.62% (-0.15%)	224 (-1)	147 (-1)	77	👎
github.com/mark3labs/mcp-go/client/transport/inprocess.go	21.57% (+21.57%)	51 (+9)	11 (+11)	40 (-2)	🌟
github.com/mark3labs/mcp-go/client/transport/sse.go	71.01% (ø)	238	169	69
github.com/mark3labs/mcp-go/client/transport/stdio.go	81.71% (+0.99%)	175 (+9)	143 (+9)	32	👍
github.com/mark3labs/mcp-go/client/transport/streamable_http.go	72.79% (-0.15%)	305 (+2)	222 (+1)	83 (+1)	👎

Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code.

Changed unit test files

• github.com/mark3labs/mcp-go/client/issue583_test.go
• github.com/mark3labs/mcp-go/client/transport/idempotent_all_transports_test.go
• github.com/mark3labs/mcp-go/client/transport/stdio_idempotent_test.go