-
Notifications
You must be signed in to change notification settings - Fork 569
fix(srv/stream): properly remove sessionTool to prevent memory leak #365
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThe session deletion logic in the Changes
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
return | ||
} | ||
|
||
s.mu.Lock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could keep code below only and remove code above, delete
is an idempotent operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for my delayed response — and you’re absolutely right that delete
is idempotent and safe even when the key does not exist.
That said, the reason we added the double-checked locking is because we noticed that most streamableHttpSessions
don’t call SetSessionTools()
to customize session tools or override global tools, meaning that in many cases the sessionID
won’t be present in sessionToolsStore.tools(map)
at all. With this in mind, the early RLock
check helps us avoid acquiring an exclusive lock unnecessarily, which could reduce write lock contention and context switching under high server load — especially in a read-heavy environment..
Do you think this trade-off makes sense? If you feel this is an over-optimization, I’d be happy to simplify the code as you suggested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think keeping the code below is good enough, it's very readable. and the pre-check only make sense if the same sessionID is deleted multiple times, but that’s a rare case (or should not happen)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
Description
Fix two potential issues in StreamableHTTPServer's func
sessionToolsStore
that may lead to performance degradation or memory leak over time:Previously, when a client session was deleted (via
handleDelete
), we simply set its entry insessionToolsStore
to nil, but did not remove the key from the internal map. Over time, this could cause the internal map to grow unbounded, especially on long-running servers with many short-lived sessions.Even for sessions that never registered/customized any tools, we would still insert a
sessionID → nil
entry during deletion, which is unnecessary and wasteful.Fix
delete(sessionID string)
method tosessionToolsStore
that safely removes the key from the internal map.Proof


Type of Change
Checklist
MCP Spec Compliance
Summary by CodeRabbit