fix: Add missing session cleanup to the StreamableHTTPServer DELETE handler #667
Conversation
WalkthroughOn StreamableHTTP DELETE handling, the server now removes the session from its in-memory Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 1
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)
657-682: Guard against empty session ID.If a client sends
DELETEwithoutMcp-Session-Id, the handler will delete session-scoped stores for""(Line 659). Consider returning400 Bad RequestwhensessionID == ""to prevent unintended cleanup of non-existent sessions.func (s *StreamableHTTPServer) handleDelete(w http.ResponseWriter, r *http.Request) { // delete request terminate the session sessionID := r.Header.Get(HeaderKeySessionID) + if sessionID == "" { + http.Error(w, "Missing session ID", http.StatusBadRequest) + return + } sessionIdManager := s.sessionIdManagerResolver.ResolveSessionIdManager(r)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/streamable_http.go(1 hunks)server/streamable_http_test.go(2 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.goserver/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 (2)
📚 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-03-04T07:00:57.111Z
Learnt from: xinwo
Repo: mark3labs/mcp-go PR: 35
File: mcp/tools.go:0-0
Timestamp: 2025-03-04T07:00:57.111Z
Learning: The Tool struct in the mark3labs/mcp-go project should handle both InputSchema and RawInputSchema consistently between MarshalJSON and UnmarshalJSON methods, even though the tools response from MCP server typically doesn't contain rawInputSchema.
Applied to files:
server/streamable_http_test.go
🧬 Code graph analysis (1)
server/streamable_http_test.go (4)
server/session.go (1)
ClientSession(12-21)server/hooks.go (1)
Hooks(94-121)client/transport/constants.go (1)
HeaderKeySessionID(5-5)server/constants.go (1)
HeaderKeySessionID(5-5)
🔇 Additional comments (1)
server/streamable_http_test.go (1)
18-20: Good move switching new assertions to testify.
Matches AGENTS.md guidance. Based on learnings, ...
|
@cnnrznn looks good. Could you fix the merge conflict? After that, will merge. |
|
@ezynda3 done. Thanks for the review :) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
server/streamable_http.go (1)
701-702: LGTM! This correctly fixes the missing UnregisterSession hook invocation.The additions properly clean up the session by removing it from
activeSessionsand callingUnregisterSession, which ensures OnUnregisterSession hooks are triggered as reported in issue #651. The cleanup order matches the pattern used inhandleGet(lines 556-557).Optional: Consider logging UnregisterSession errors for better observability
While the current approach is consistent with the defer pattern at line 556, logging any errors would improve observability and match the pattern used for
RegisterSessionat line 508.s.activeSessions.Delete(sessionID) -s.server.UnregisterSession(r.Context(), sessionID) +if err := s.server.UnregisterSession(r.Context(), sessionID); err != nil { + s.logger.Errorf("Failed to unregister session %s: %v", sessionID, err) +}
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/streamable_http.go(1 hunks)server/streamable_http_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/streamable_http_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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.go
|
@cnnrznn there's a typo in your test I think as well as some linting issues that need fixing |
|
Apologies. Linting is fixed.
Tests pass locally: `go test ./... -race`
…On Fri, Dec 19, 2025, 7:22 AM Ed Zynda ***@***.***> wrote:
*ezynda3* left a comment (mark3labs/mcp-go#667)
<#667 (comment)>
@cnnrznn <https://github.com/cnnrznn> there's a typo in your test I think
as well as some linting issues that need fixing
—
Reply to this email directly, view it on GitHub
<#667 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACDYUBIXLWHNQTONLFRBFR34CPUZLAVCNFSM6AAAAACOZWIDHKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTMNZUHA4DCMRUGA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
3 participants
Description
This PR adds two missing session cleanup calls to the streamable http transport's handleDelete method.
UnregisterSession()methodFixes #651
Type of Change
Checklist
Additional Information
When writing my test, I noticed an inconsistency between the existing assertion style and AGENTS.md.
AGENTS.md tells agents to use assert/require. The existing tests use
t.Error().Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.