fix: create StatelessGeneratingSessionIdManager to fix multi-instance deployments #641
Conversation
…ce deployments Fixes #636 In commit da6f722, the default session ID manager was changed from StatelessSessionIdManager to InsecureStatefulSessionIdManager, which broke multi-instance deployments without sticky sessions by causing Invalid session ID errors when requests were routed to different server instances. This change restores the previous default behavior: - Default session manager is now StatelessSessionIdManager (no session validation) - Multi-instance deployments work without requiring sticky sessions - Added WithStateful() option for explicit stateful session management - Updated all nil fallbacks to use stateless manager - Updated tests to verify new default behavior - Updated documentation to reflect the change Backward compatibility is maintained while fixing the production deployment issue.
WalkthroughDefaults for session management were changed to stateless by default and a new public option Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/streamable_http.go (1)
193-204: Default resolver change to StatelessSessionIdManager is aligned with the bugfix but impacts tests and existing behaviorSetting
sessionIdManagerResolvertoNewDefaultSessionIdManagerResolver(&StatelessSessionIdManager{})makes the server stateless by default and fixes the multi-instance “Invalid session ID” issue, which is the stated goal of the PR.However, several tests still rely on stateful behavior from the default server (no options), for example:
TestStreamableHTTP_POST_SendAndReceive(“initialize” and “Invalid session id” subtests) expect a non-empty session ID header and 400 for bogus IDs.TestStreamableHTTP_SessionValidationexpects 400/404 responses for invalid/terminated session IDs using a defaultNewTestStreamableHTTPServer(mcpServer).TestStreamableHTTP_SendNotificationToSpecificClientexpects a session ID header from an initialize POST on the default server.With this new default, those scenarios will only hold when the server is created with
WithStateful(true). Consider updating those tests (and any similar ones) to construct the server withWithStateful(true)when they are explicitly verifying stateful behavior, and leaving default, option-less servers to assert stateless semantics.- server := NewTestStreamableHTTPServer(mcpServer) + server := NewTestStreamableHTTPServer(mcpServer, WithStateful(true))(Apply this pattern in the tests that assert stateful semantics such as non-empty session IDs or “Invalid session ID” errors.)
🧹 Nitpick comments (1)
server/streamable_http.go (1)
82-90: WithStateful option API looks correct, but only acts whenstateful == true
WithStateful(true)cleanly switches the resolver to anInsecureStatefulSessionIdManager, and the doc comment correctly warns about sticky sessions. Note thatWithStateful(false)is a no-op, so callers cannot “turn off” stateful mode with a laterfalse—they must rely on option order (or avoid passing it). If you want a strictly symmetric API later, you could consider documenting this explicitly or using an enum-like config.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/streamable_http.go(4 hunks)server/streamable_http_test.go(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Order imports: standard library first, then third-party, then local packages (goimports enforces this)
Follow Go naming conventions: exported identifiers in PascalCase; unexported in camelCase; acronyms uppercase (HTTP, JSON, MCP)
Error handling: return sentinel errors, wrap with fmt.Errorf("context: %w", err), and check with errors.Is/As
Prefer explicit types and strongly-typed structs; avoid using any except where protocol flexibility is required (e.g., Arguments any)
All exported types and functions must have GoDoc comments starting with the identifier name; avoid inline comments unless necessary
Functions that are handlers or long-running must accept context.Context as the first parameter
Ensure thread safety for shared state using sync.Mutex and document thread-safety requirements in comments
For JSON: use json struct tags with omitempty for optional fields; use json.RawMessage for flexible/deferred parsing
Files:
server/streamable_http_test.goserver/streamable_http.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Testing: use testify/assert and testify/require
Write table-driven tests using a tests := []struct{ name, ... } pattern
Go test files must end with _test.go
Files:
server/streamable_http_test.go
🧬 Code graph analysis (1)
server/streamable_http_test.go (1)
server/streamable_http.go (2)
NewStreamableHTTPServer(194-211)WithStateful(84-90)
⏰ 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). (4)
- GitHub Check: coverage
- GitHub Check: test
- GitHub Check: coverage
- GitHub Check: test
🔇 Additional comments (6)
server/streamable_http.go (2)
54-66: Nil manager now falls back to stateless (desired for multi-instance safety)Using
StatelessSessionIdManageras the fallback whenWithSessionIdManager(nil)is passed aligns with the new stateless-by-default story and avoids reintroducing local state when callers misconfigure this option. The behavior and comments are consistent with the PR goal.
72-80: Nil resolver now falls back to stateless (consistent with other entry points)Having
WithSessionIdManagerResolver(nil)wrap aStatelessSessionIdManagerviaNewDefaultSessionIdManagerResolverkeeps the server safe-by-default even when callers passnil. This matches the new default and avoids surprising reversion to stateful mode.server/streamable_http_test.go (4)
1863-1877: Stateless default preserved when WithStateLess(false) is usedThe expectations here are correct:
WithStateLess(false)should be a no-op, leaving the server’s default resolver asStatelessSessionIdManager, and asserting an empty session ID fromGenerate()verifies that.
1926-1943: Nil resolver path now explicitly asserts stateless fallbackThis test correctly verifies that
WithSessionIdManagerResolver(nil)results in a non-nil resolver that uses stateless semantics (empty session IDs). That matches the new implementation and protects against accidental reintroduction of stateful behavior vianil.
1945-1962: Nil manager path now asserts stateless fallbackThe expectations that
WithSessionIdManager(nil)yields a resolver whoseGenerate()returns""are consistent with the code change and ensure the fallback is stateless rather than stateful.
1964-1984: Chained nil options correctly tested for stateless fallbackThe “Multiple nil options fall back safely” test confirms that
WithSessionIdManager(nil)followed byWithSessionIdManagerResolver(nil)still lands on a functioning, stateless manager. This is a good regression test for the interplay of these options.
… deployments Fixes #636 The original fix was too aggressive - changing to completely stateless broke existing functionality that expects session IDs to be generated. New approach: - Created StatelessGeneratingSessionIdManager that generates session IDs but only validates format (not existence locally) - This fixes multi-instance deployments while maintaining compatibility - Updated tests to reflect new behavior - Session termination now requires explicit stateful mode This solves the production issue where requests routed to different instances failed with 'Invalid session ID' errors while preserving the expected session ID generation behavior.
ezynda3
changed the title
…essionIdManager
As requested, updated NewDefaultSessionIdManagerResolver to fall back to
StatelessSessionIdManager instead of StatelessGeneratingSessionIdManager
when manager is nil.
This change affects:
- NewDefaultSessionIdManagerResolver(nil) fallback
- WithSessionIdManager(nil) fallback
- WithSessionIdManagerResolver(nil) fallback
Updated corresponding tests to expect stateless behavior:
- Generate() returns empty string ("")
- Validate() accepts any session ID without error
The server default still uses StatelessGeneratingSessionIdManager for
backward compatibility, but nil fallbacks now use truly stateless behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
server/streamable_http.go (1)
82-90: Consider clarifyingWithStateful(false)behavior.The function only acts when
statefulis true; passingfalseis a no-op since the default is already stateless. While this is likely intentional, it could be clearer to readers.Consider adding a comment to clarify:
// WithStateful enables stateful session management using InsecureStatefulSessionIdManager. -// This requires sticky sessions in multi-instance deployments. +// When true, enables stateful session management (requires sticky sessions in multi-instance deployments). +// When false, this option has no effect; the default remains stateless (StatelessGeneratingSessionIdManager). func WithStateful(stateful bool) StreamableHTTPOption {Alternatively, you could make the no-op case explicit:
func WithStateful(stateful bool) StreamableHTTPOption { return func(s *StreamableHTTPServer) { if stateful { s.sessionIdManagerResolver = NewDefaultSessionIdManagerResolver(&InsecureStatefulSessionIdManager{}) + } else { + // Explicitly retain default stateless behavior - no action needed } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/streamable_http.go(5 hunks)server/streamable_http_test.go(8 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Order imports: standard library first, then third-party, then local packages (goimports enforces this)
Follow Go naming conventions: exported identifiers in PascalCase; unexported in camelCase; acronyms uppercase (HTTP, JSON, MCP)
Error handling: return sentinel errors, wrap with fmt.Errorf("context: %w", err), and check with errors.Is/As
Prefer explicit types and strongly-typed structs; avoid using any except where protocol flexibility is required (e.g., Arguments any)
All exported types and functions must have GoDoc comments starting with the identifier name; avoid inline comments unless necessary
Functions that are handlers or long-running must accept context.Context as the first parameter
Ensure thread safety for shared state using sync.Mutex and document thread-safety requirements in comments
For JSON: use json struct tags with omitempty for optional fields; use json.RawMessage for flexible/deferred parsing
Files:
server/streamable_http_test.goserver/streamable_http.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Testing: use testify/assert and testify/require
Write table-driven tests using a tests := []struct{ name, ... } pattern
Go test files must end with _test.go
Files:
server/streamable_http_test.go
🧠 Learnings (4)
📚 Learning: 2025-10-13T09:35:20.180Z
Learnt from: CR
Repo: mark3labs/mcp-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-13T09:35:20.180Z
Learning: Applies to **/*_test.go : Testing: use testify/assert and testify/require
Applied to files:
server/streamable_http_test.go
📚 Learning: 2025-06-23T11:10:42.948Z
Learnt from: floatingIce91
Repo: mark3labs/mcp-go PR: 401
File: server/server.go:1082-1092
Timestamp: 2025-06-23T11:10:42.948Z
Learning: In Go MCP server, ServerTool.Tool field is only used for tool listing and indexing, not for tool execution or middleware. During handleToolCall, only the Handler field is used, so dynamic tools don't need the Tool field populated.
Applied to files:
server/streamable_http_test.go
📚 Learning: 2025-03-04T06:59:43.882Z
Learnt from: xinwo
Repo: mark3labs/mcp-go PR: 35
File: mcp/tools.go:107-137
Timestamp: 2025-03-04T06:59:43.882Z
Learning: Tool responses from the MCP server shouldn't contain RawInputSchema, which is why the UnmarshalJSON method for the Tool struct is implemented to handle only the structured InputSchema format.
Applied to files:
server/streamable_http_test.go
📚 Learning: 2025-04-21T21:26:32.945Z
Learnt from: octo
Repo: mark3labs/mcp-go PR: 149
File: mcptest/mcptest.go:0-0
Timestamp: 2025-04-21T21:26:32.945Z
Learning: In the mcptest package, prefer returning errors from helper functions rather than calling t.Fatalf() directly, giving callers flexibility in how to handle errors.
Applied to files:
server/streamable_http_test.go
🧬 Code graph analysis (1)
server/streamable_http_test.go (2)
server/streamable_http.go (3)
NewTestStreamableHTTPServer(1352-1356)WithStateful(84-90)NewStreamableHTTPServer(194-211)server/constants.go (1)
HeaderKeySessionID(5-5)
⏰ 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). (4)
- GitHub Check: coverage
- GitHub Check: test
- GitHub Check: test
- GitHub Check: coverage
🔇 Additional comments (9)
server/streamable_http.go (5)
54-66: LGTM! Nil manager fallback is correct.The nil guard correctly defaults to
StatelessGeneratingSessionIdManager, and the updated comment clearly documents the new default behavior. This aligns with the PR's goal of restoring stateless-by-default behavior for multi-instance deployments.
68-80: LGTM! Consistent nil handling.Follows the same pattern as
WithSessionIdManager, correctly defaulting toStatelessGeneratingSessionIdManagerwhen nil.
194-211: LGTM! Default correctly restored to stateless behavior.The default initialization with
StatelessGeneratingSessionIdManagerdirectly addresses the multi-instance deployment issue described in #636. This generates unique session IDs while avoiding local state that breaks deployments without sticky sessions.
1254-1260: LGTM! Nil fallback uses generating manager (better than past suggestion).The nil guard correctly defaults to
StatelessGeneratingSessionIdManager. Note that this generates non-empty, prefixed session IDs (unlikeStatelessSessionIdManagerwhich returns empty strings). This is actually better than the past review comment's suggestion because it:
- Provides format validation (prefix + UUID check)
- Generates unique IDs useful for tracking/logging
- Maintains consistency with session ID format expectations
The test at lines 1822-1829 correctly expects this behavior.
1283-1305: LGTM! Excellent implementation for multi-instance deployments.The
StatelessGeneratingSessionIdManagerstrikes the right balance:
- Generate: Creates unique session IDs with consistent format (
mcp-session-+ UUID), enabling tracking and logging while remaining stateless.- Validate: Only checks format (prefix + valid UUID), not existence—this is critical for multi-instance deployments where session state isn't shared.
- Terminate: No-op is correct since there's no local state to clean up.
This design allows session IDs to work across multiple server instances without requiring sticky sessions or shared state, directly addressing the issue in #636.
Thread-safety: No mutable state, so no synchronization needed. ✓
server/streamable_http_test.go (4)
1407-1452: LGTM! Test correctly validates new stateless-by-default behavior.The test rename and logic update accurately reflect the behavior change:
- Old default (InsecureStatefulSessionIdManager): Rejected
mcp-session-ffffffff-ffff-ffff-ffff-ffffffffffffbecause the session wasn't tracked locally → 400- New default (StatelessGeneratingSessionIdManager): Accepts properly-formatted session IDs regardless of local tracking → 200
The response parsing (lines 1432-1451) correctly verifies that the tool call executes and returns a valid result, confirming end-to-end functionality with the new manager.
1527-1603: LGTM! Correctly tests stateful session termination with explicit opt-in.This test appropriately uses
WithStateful(true)to test stateful-specific behavior (session termination tracking). The test flow is clear:
- Initialize session with stateful manager
- Terminate the session
- Verify subsequent requests return 404
This addresses the past review comment's recommendation to use
WithStateful(true)for tests that explicitly require stateful behavior (tracking terminated sessions).
2067-2087: LGTM! Excellent test coverage forWithStatefuloption.This test clearly validates the new
WithStateful(true)behavior:
- ✓ Stateful manager generates non-empty session IDs with correct prefix
- ✓ Stateful manager rejects unknown session IDs (lines 2083-2086)
The second assertion is particularly important—it distinguishes stateful behavior (local tracking + rejection of unknown IDs) from the default stateless-generating behavior (format-only validation).
1807-1830: LGTM! Nil handling tests correctly updated for generating manager.All nil fallback tests now correctly expect
StatelessGeneratingSessionIdManagerbehavior:
- ✓ Non-empty session IDs with
mcp-session-prefix- ✓ Valid UUID format verification
These updates align with the design decision to use a generating manager (which provides format validation and unique IDs) rather than a purely stateless manager (empty IDs). The tests comprehensively cover nil handling across all configuration options:
WithSessionIdManager(nil),WithSessionIdManagerResolver(nil), and chained nil options.Note: This differs from the past review comment's suggestion (expecting empty IDs), but the generating approach is superior for production use.
Also applies to: 1902-1919, 1968-1988, 1990-2010, 2012-2035
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
server/streamable_http.go (1)
55-80: Nil fallback behavior is safe but potentially confusing.The nil fallbacks use
StatelessSessionIdManager(no ID generation), while the normal default usesStatelessGeneratingSessionIdManager(generates IDs). This creates distinct behaviors:
- Default (no options): Generates session IDs
- Explicit nil: Doesn't generate session IDs (fully stateless)
While this is tested and appears intentional as a "safe" defensive fallback, the distinction between the normal default and nil-fallback behavior could be clearer in documentation.
Consider adding a comment clarifying this distinction, for example:
// Note: When manager is nil, falls back to StatelessSessionIdManager (fully stateless, no ID generation) // as a safe defensive default. This differs from the normal default which generates IDs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/streamable_http.go(5 hunks)server/streamable_http_test.go(10 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Order imports: standard library first, then third-party, then local packages (goimports enforces this)
Follow Go naming conventions: exported identifiers in PascalCase; unexported in camelCase; acronyms uppercase (HTTP, JSON, MCP)
Error handling: return sentinel errors, wrap with fmt.Errorf("context: %w", err), and check with errors.Is/As
Prefer explicit types and strongly-typed structs; avoid using any except where protocol flexibility is required (e.g., Arguments any)
All exported types and functions must have GoDoc comments starting with the identifier name; avoid inline comments unless necessary
Functions that are handlers or long-running must accept context.Context as the first parameter
Ensure thread safety for shared state using sync.Mutex and document thread-safety requirements in comments
For JSON: use json struct tags with omitempty for optional fields; use json.RawMessage for flexible/deferred parsing
Files:
server/streamable_http_test.goserver/streamable_http.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Testing: use testify/assert and testify/require
Write table-driven tests using a tests := []struct{ name, ... } pattern
Go test files must end with _test.go
Files:
server/streamable_http_test.go
🧠 Learnings (4)
📚 Learning: 2025-06-23T11:10:42.948Z
Learnt from: floatingIce91
Repo: mark3labs/mcp-go PR: 401
File: server/server.go:1082-1092
Timestamp: 2025-06-23T11:10:42.948Z
Learning: In Go MCP server, ServerTool.Tool field is only used for tool listing and indexing, not for tool execution or middleware. During handleToolCall, only the Handler field is used, so dynamic tools don't need the Tool field populated.
Applied to files:
server/streamable_http_test.go
📚 Learning: 2025-03-04T06:59:43.882Z
Learnt from: xinwo
Repo: mark3labs/mcp-go PR: 35
File: mcp/tools.go:107-137
Timestamp: 2025-03-04T06:59:43.882Z
Learning: Tool responses from the MCP server shouldn't contain RawInputSchema, which is why the UnmarshalJSON method for the Tool struct is implemented to handle only the structured InputSchema format.
Applied to files:
server/streamable_http_test.go
📚 Learning: 2025-04-21T21:26:32.945Z
Learnt from: octo
Repo: mark3labs/mcp-go PR: 149
File: mcptest/mcptest.go:0-0
Timestamp: 2025-04-21T21:26:32.945Z
Learning: In the mcptest package, prefer returning errors from helper functions rather than calling t.Fatalf() directly, giving callers flexibility in how to handle errors.
Applied to files:
server/streamable_http_test.go
📚 Learning: 2025-06-30T07:13:17.052Z
Learnt from: ezynda3
Repo: mark3labs/mcp-go PR: 461
File: server/sampling.go:22-26
Timestamp: 2025-06-30T07:13:17.052Z
Learning: In the mark3labs/mcp-go project, the MCPServer.capabilities field is a struct value (serverCapabilities), not a pointer, so it cannot be nil and doesn't require nil checking. Only pointer fields within the capabilities struct should be checked for nil.
Applied to files:
server/streamable_http_test.go
🧬 Code graph analysis (1)
server/streamable_http_test.go (2)
server/streamable_http.go (3)
NewTestStreamableHTTPServer(1352-1356)WithStateful(84-90)NewStreamableHTTPServer(194-211)server/constants.go (1)
HeaderKeySessionID(5-5)
⏰ 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). (3)
- GitHub Check: coverage
- GitHub Check: test
- GitHub Check: test
🔇 Additional comments (10)
server/streamable_http_test.go (6)
1407-1451: LGTM! Test correctly reflects stateless generating behavior.The test rename and expectation changes accurately reflect the new default behavior where
StatelessGeneratingSessionIdManagervalidates session ID format but not existence, allowing properly formatted session IDs to work across instances.
1527-1603: LGTM! Stateful termination test provides essential coverage.This test correctly opts into stateful mode with
WithStateful(true)and validates that termination semantics work as expected when local session tracking is enabled. This is important coverage for users who need explicit session termination.
1822-1835: LGTM! Nil fallback correctly expects fully stateless behavior.The updated expectations correctly reflect that
nilmanager fallback usesStatelessSessionIdManager(returning empty strings), which is a safe defensive default distinct from the normal defaultStatelessGeneratingSessionIdManager.
1913-1924: LGTM! Test correctly validates default generating behavior.The updated expectations properly reflect that
WithStateLess(false)maintains the defaultStatelessGeneratingSessionIdManager, which generates prefixed UUID-based session IDs while validating only format (not existence).
1974-2032: LGTM! Nil handling tests are thorough and correct.These tests comprehensively verify that
nilinputs fall back toStatelessSessionIdManager(fully stateless, no ID generation) as a safe default. The behavior is consistent across all nil-handling scenarios.Minor note: The comments alternate between "fallback" and "default" terminology when referring to the nil case, but the intent is clear from context.
2064-2084: LGTM! WithStateful test validates opt-in stateful behavior.This test correctly exercises the new
WithStateful(true)option and verifies that stateful mode generates session IDs and validates their existence (rejecting unknown IDs), which is the key distinction from the default stateless generating mode.server/streamable_http.go (4)
82-90: LGTM! WithStateful option provides clear opt-in for stateful mode.The new option cleanly enables stateful session management when needed, and the documentation correctly warns users about the sticky session requirement for multi-instance deployments.
193-210: LGTM! Default change fixes multi-instance session handling.Changing the default from
InsecureStatefulSessionIdManagertoStatelessGeneratingSessionIdManagercorrectly addresses the multi-instance deployment issue. Session IDs are now generated and validated by format only (not local existence), enabling cross-instance compatibility without sticky sessions.
1255-1259: LGTM! Nil fallback provides safe defensive default.The nil fallback to
StatelessSessionIdManageris consistent with other nil-handling code and provides a safe fully-stateless default when no manager is explicitly configured.
1283-1305: Add GoDoc comment for the exported type.The implementation of
StatelessGeneratingSessionIdManageris correct and achieves the PR's goal of format-only validation for cross-instance compatibility. However, the exported type is missing a GoDoc comment.As per coding guidelines, add a GoDoc comment starting with the type name:
+// StatelessGeneratingSessionIdManager generates UUID-based session IDs and validates +// their format without tracking them locally. This enables cross-instance session handling +// without requiring sticky sessions or shared state. type StatelessGeneratingSessionIdManager struct{}Based on coding guidelines
⛔ Skipped due to learnings
Learnt from: CR Repo: mark3labs/mcp-go PR: 0 File: AGENTS.md:0-0 Timestamp: 2025-10-13T09:35:20.180Z Learning: Applies to **/*.go : All exported types and functions must have GoDoc comments starting with the identifier name; avoid inline comments unless necessaryLearnt from: CR Repo: mark3labs/mcp-go PR: 0 File: AGENTS.md:0-0 Timestamp: 2025-10-13T09:35:20.180Z Learning: Applies to **/*.go : Ensure thread safety for shared state using sync.Mutex and document thread-safety requirements in comments
As requested in the CodeRabbit review, updated tests that expect stateful behavior to explicitly use WithStateful(true) instead of relying on the old default behavior. Updated tests: - TestStreamableHTTP_POST_SendAndReceive: expects session IDs in headers and 400 for invalid IDs - TestStreamableHttpResourceGet: expects session ID in header - TestStreamableHTTP_SessionWithLogging: expects session ID in header - TestStreamableHTTP_SendNotificationToSpecificClient: both subtests expect session IDs in headers This ensures tests explicitly opt into stateful mode when needed, while the default remains stateless for multi-instance deployments.
- Updated coverage job condition to run on push to main branch in addition to PRs - Added 30-day artifact retention to ensure coverage artifacts are available for comparison - This fixes the coverage triggers that were failing due to missing baseline artifacts Now coverage will: 1. Run on main branch pushes to create baseline artifacts 2. Run on PRs to compare against baseline and ensure coverage doesn't decrease 3. Keep artifacts available for 30 days for proper comparison
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
server/streamable_http_test.go (1)
1823-1836: Consider documenting nil-fallback vs default manager behaviorThe tests reveal two different stateless managers in use:
- Normal default (line 1914):
StatelessGeneratingSessionIdManagergenerates non-empty UUID-based session IDs (lines 1919-1925)- Nil fallback (line 1810, 1823-1827): Falls back to
StatelessSessionIdManagerwhich returns empty session IDsWhile this defensive nil-handling is reasonable, the distinction between "default stateless" and "nil-fallback stateless" could confuse users. The tests are correct, but consider adding comments explaining when each manager is used.
Also applies to: 1914-1925
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/ci.yml(2 hunks)server/streamable_http_test.go(15 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Order imports: standard library first, then third-party, then local packages (goimports enforces this)
Follow Go naming conventions: exported identifiers in PascalCase; unexported in camelCase; acronyms uppercase (HTTP, JSON, MCP)
Error handling: return sentinel errors, wrap with fmt.Errorf("context: %w", err), and check with errors.Is/As
Prefer explicit types and strongly-typed structs; avoid using any except where protocol flexibility is required (e.g., Arguments any)
All exported types and functions must have GoDoc comments starting with the identifier name; avoid inline comments unless necessary
Functions that are handlers or long-running must accept context.Context as the first parameter
Ensure thread safety for shared state using sync.Mutex and document thread-safety requirements in comments
For JSON: use json struct tags with omitempty for optional fields; use json.RawMessage for flexible/deferred parsing
Files:
server/streamable_http_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Testing: use testify/assert and testify/require
Write table-driven tests using a tests := []struct{ name, ... } pattern
Go test files must end with _test.go
Files:
server/streamable_http_test.go
🧠 Learnings (4)
📚 Learning: 2025-06-23T11:10:42.948Z
Learnt from: floatingIce91
Repo: mark3labs/mcp-go PR: 401
File: server/server.go:1082-1092
Timestamp: 2025-06-23T11:10:42.948Z
Learning: In Go MCP server, ServerTool.Tool field is only used for tool listing and indexing, not for tool execution or middleware. During handleToolCall, only the Handler field is used, so dynamic tools don't need the Tool field populated.
Applied to files:
server/streamable_http_test.go
📚 Learning: 2025-03-04T06:59:43.882Z
Learnt from: xinwo
Repo: mark3labs/mcp-go PR: 35
File: mcp/tools.go:107-137
Timestamp: 2025-03-04T06:59:43.882Z
Learning: Tool responses from the MCP server shouldn't contain RawInputSchema, which is why the UnmarshalJSON method for the Tool struct is implemented to handle only the structured InputSchema format.
Applied to files:
server/streamable_http_test.go
📚 Learning: 2025-04-21T21:26:32.945Z
Learnt from: octo
Repo: mark3labs/mcp-go PR: 149
File: mcptest/mcptest.go:0-0
Timestamp: 2025-04-21T21:26:32.945Z
Learning: In the mcptest package, prefer returning errors from helper functions rather than calling t.Fatalf() directly, giving callers flexibility in how to handle errors.
Applied to files:
server/streamable_http_test.go
📚 Learning: 2025-06-30T07:13:17.052Z
Learnt from: ezynda3
Repo: mark3labs/mcp-go PR: 461
File: server/sampling.go:22-26
Timestamp: 2025-06-30T07:13:17.052Z
Learning: In the mark3labs/mcp-go project, the MCPServer.capabilities field is a struct value (serverCapabilities), not a pointer, so it cannot be nil and doesn't require nil checking. Only pointer fields within the capabilities struct should be checked for nil.
Applied to files:
server/streamable_http_test.go
🧬 Code graph analysis (1)
server/streamable_http_test.go (2)
server/streamable_http.go (3)
NewTestStreamableHTTPServer(1352-1356)WithStateful(84-90)NewStreamableHTTPServer(194-211)client/transport/constants.go (1)
HeaderKeySessionID(5-5)
⏰ 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). (4)
- GitHub Check: coverage
- GitHub Check: test
- GitHub Check: test
- GitHub Check: coverage
🔇 Additional comments (11)
.github/workflows/ci.yml (2)
22-22: Good: Extend coverage tracking to main branch.The conditional correctly triggers the coverage job on pull requests and on pushes to the main branch, ensuring consistent coverage metrics for the default branch.
41-41: Good: Set reasonable artifact retention.The 30-day retention period for coverage artifacts provides sufficient history while managing storage costs.
server/streamable_http_test.go (9)
128-128: LGTM: Appropriate use of WithStateful(true)This test validates session ID generation, header propagation, and invalid session rejection, all of which require stateful session tracking. The addition of
WithStateful(true)correctly enables these behaviors.
598-598: LGTM: Stateful mode required for session-specific stateBoth tests correctly use
WithStateful(true):
- Line 598: Session-specific resources require local session tracking
- Line 1018: Session-specific log levels require local session tracking
Also applies to: 1018-1018
1408-1453: Good: Test updated to reflect stateless format-only validationThe test correctly validates the new stateless behavior where session IDs are validated by format only, not existence. A properly formatted session ID (line 1421) now returns HTTP 200 OK with valid response content, rather than being rejected. This aligns with the PR's goal of enabling cross-instance deployments without sticky sessions.
1528-1604: LGTM: Comprehensive test for stateful session terminationThis test correctly validates stateful-specific behavior:
- Explicitly enables stateful mode (line 1531)
- Initializes and obtains a valid session ID
- Terminates the session via DELETE
- Verifies that subsequent requests with the terminated session ID return 404
This is essential coverage since the default (stateless) mode doesn't track termination.
1978-2032: LGTM: Comprehensive nil-handling coverageThese tests thoroughly validate defensive nil-handling across different option types:
- Nil resolver (lines 1978-1991)
- Nil manager (lines 1994-2010)
- Chained nil options (lines 2013-2032)
All correctly expect fallback to
StatelessSessionIdManager(empty session IDs). The test coverage is solid.
2065-2085: LGTM: Clear validation of WithStateful optionThis test effectively validates the
WithStateful(true)option:
- Generates non-empty session IDs with the correct prefix (lines 2072-2078)
- Validates session existence, not just format (lines 2080-2084)
The test clearly demonstrates the difference between stateful (existence checking) and default stateless (format checking) behavior.
2104-2104: LGTM: Stateful mode required for session-specific notificationsBoth tests correctly use
WithStateful(true)sinceSendNotificationToSpecificClientrequires a local session registry to route notifications to specific session IDs.Also applies to: 2175-2175
320-502: Excellent test coverage for stateful/stateless session managementThe test suite comprehensively validates the new session management behavior:
✅ Strengths:
- Clear separation of stateful vs stateless test cases
- Tests cover format-only validation (stateless) and existence validation (stateful)
- Good coverage of nil-handling edge cases
- Tests align with PR objectives (cross-instance compatibility)
- Explicit
WithStateful(true)usage where local session tracking is required
⚠️ One concern:
- Line 322 uses
WithStateLess(true)which may not exist (needs verification - see separate comment)Also applies to: 598-598, 1018-1018, 1408-1453, 1528-1604, 2065-2085, 2104-2104, 2175-2175
320-502: No issues foundThe
WithStateLessoption is properly defined inserver/streamable_http.go:46. The test code on line 322 is valid and will compile correctly.
Description
Created StatelessGeneratingSessionIdManager to fix multi-instance deployments that were broken after commit da6f722. The change to InsecureStatefulSessionIdManager caused "Invalid session ID" errors in deployments without sticky sessions because it validated session existence locally.
The new default manager generates session IDs but only validates format (not existence locally), which fixes multi-instance deployments while maintaining backward compatibility.
Fixes #636
Type of Change
Checklist
Additional Information
Changes Made:
Impact:
Technical Details:
Migration:
No migration required for most users. Only users who need local session termination should use WithStateful(true).
Summary by CodeRabbit
New Features
Changes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.