feat(mcp): Add SessionIdManagerResolver interface for request-based session management #626
Conversation
|
Warning Rate limit exceeded@aradyaron has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 13 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughAdds a per-request session ID manager resolver: new 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)
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)
55-57: Fix stale doc and nil-guard manager → resolver
- The comment references SimpleStatefulSessionIdGenerator; code uses InsecureStatefulSessionIdManager.
- Passing a nil manager produces a resolver returning nil -> panic on Generate/Validate/Terminate.
Apply:
-// By default, the server will use SimpleStatefulSessionIdGenerator, which generates -// session ids with uuid, and it's insecure. +// By default, the server uses InsecureStatefulSessionIdManager (UUID-based; insecure). func WithSessionIdManager(manager SessionIdManager) StreamableHTTPOption { return func(s *StreamableHTTPServer) { - s.sessionIdManagerResolver = NewDefaultSessionIdManagerResolver(manager) + if manager == nil { + s.sessionIdManagerResolver = NewDefaultSessionIdManagerResolver(&InsecureStatefulSessionIdManager{}) + return + } + s.sessionIdManagerResolver = NewDefaultSessionIdManagerResolver(manager) } }Also applies to: 57-62
🧹 Nitpick comments (3)
server/streamable_http.go (2)
64-67: Clarify option precedence (last-wins) in docsCurrent behavior is order-dependent. Make docs explicit to avoid surprises.
-// Notice: it will override both WithStateLess and WithSessionIdManager options. +// Note: Options are applied in order; the last one wins. If combined with +// WithStateLess or WithSessionIdManager, whichever is applied last takes effect.
1193-1197: Mitigate unbounded growth of terminated setInsecureStatefulSessionIdManager.terminated grows without bounds. Consider TTL-based eviction or periodic compaction to avoid memory bloat under churn.
Also applies to: 1223-1233
server/streamable_http_test.go (1)
1772-1905: Integration coverage looks solid; add nil-guard casesAdd tests to ensure passing nil to WithSessionIdManager and WithSessionIdManagerResolver falls back safely (after implementing guards).
@@ func TestSessionIdManagerResolver_Integration(t *testing.T) { @@ t.Run("Nil manager falls back safely", func(t *testing.T) { mcpServer := NewMCPServer("test-server", "1.0.0") - // requires nil-guard in WithSessionIdManager + // requires nil-guard in WithSessionIdManager srv := NewTestStreamableHTTPServer(mcpServer, WithSessionIdManager(nil)) defer srv.Close() resp, err := postJSON(srv.URL, initRequest) if err != nil { t.Fatalf("init failed: %v", err) } if resp.StatusCode != http.StatusOK { t.Fatalf("expected 200, got %d", resp.StatusCode) } _ = resp.Body.Close() }) t.Run("Nil resolver falls back safely", func(t *testing.T) { mcpServer := NewMCPServer("test-server", "1.0.0") - // requires nil-guard in WithSessionIdManagerResolver + // requires nil-guard in WithSessionIdManagerResolver srv := NewTestStreamableHTTPServer(mcpServer, WithSessionIdManagerResolver(nil)) defer srv.Close() resp, err := postJSON(srv.URL, initRequest) if err != nil { t.Fatalf("init failed: %v", err) } if resp.StatusCode != http.StatusOK { t.Fatalf("expected 200, got %d", resp.StatusCode) } _ = resp.Body.Close() }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/streamable_http.go(9 hunks)server/streamable_http_test.go(1 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 (2)
server/streamable_http_test.go (2)
server/streamable_http.go (8)
InsecureStatefulSessionIdManager(1194-1197)NewDefaultSessionIdManagerResolver(1166-1168)StatelessSessionIdManager(1176-1176)NewStreamableHTTPServer(175-192)WithSessionIdManagerResolver(67-71)WithSessionIdManager(58-62)DefaultSessionIdManagerResolver(1161-1163)WithStateLess(46-52)server/server.go (1)
NewMCPServer(337-365)
server/streamable_http.go (3)
server/http_transport_options.go (1)
HTTPContextFunc(11-11)util/logger.go (1)
Logger(8-11)server/constants.go (1)
HeaderKeySessionID(5-5)
🔇 Additional comments (2)
server/streamable_http.go (1)
319-336: Resolver usage is applied consistently across POST/DELETE/sampling pathsPer-request resolution looks correct and maintains stateless/stateful semantics. Nice.
Also applies to: 621-626, 672-675
server/streamable_http_test.go (1)
1696-1769: Good unit coverage for DefaultSessionIdManagerResolverCovers basic resolution, stateless behavior, multi-call consistency, and nil request. LGTM.
aradyaron
changed the title
2 participants
Description
Adds a new
SessionIdManagerResolverinterface to enable request-based session ID management strategies in the StreamableHTTP server. Replaces directSessionIdManagerusage with resolver pattern while maintaining full backward compatibility.Fixes #625
Type of Change
Checklist
Additional Information
New Interface:
Key Changes:
DefaultSessionIdManagerResolverimplementationWithSessionIdManagerandWithStateLessto use resolversWithSessionIdManagerResolverconfiguration optionBenefits:
Summary by CodeRabbit
Refactor
Tests