-
Notifications
You must be signed in to change notification settings - Fork 497
fix(mark3labs/mcp): fix concurrent writes bug in intent capture #4362
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
base: main
Are you sure you want to change the base?
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
85d8a90 to
c353afa
Compare
e5d6064 to
44f4456
Compare
ddtrace with telemetry44f4456 to
7a43c31
Compare
c353afa to
928df14
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files🚀 New features to boost your workflow:
|
BenchmarksBenchmark execution time: 2026-01-20 17:27:31 Comparing candidate commit 7288e17 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 156 metrics, 8 unstable metrics. |
| } | ||
|
|
||
| // The server reuses tools across requests. Slices and nested objects are cloned to avoid concurrent writes. | ||
| result.Tools = slices.Clone(result.Tools) |
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.
@jboolean What are the odds of result.Tools's items being updated while cloning? It doesn't seem that we have any lock to ensure result.Tools or its items aren't modified while slices.Clone does its work.
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.
Interesting thought. Extremely low as tools are not dynamic afaik, but let me add some locking.
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.
Actually, if it is written to anywhere inside the library itself, I wouldn't be able to control that.
I had Claude do a deep investigation and it concluded that this is not an issue. What do you think:
Summary: Line 39 slices.Clone(result.Tools) is Safe
TL;DR: No other goroutine can write to result.Tools during the clone operation because each request gets its own local slice that no other goroutine has a reference to.
Key Evidence from mcp-go
Each ListTools request creates its own local tools slice:
// /Users/julian.boilen/dd/mcp-go/server/server.go:1159 func (s *MCPServer) handleListTools( ctx context.Context, id any, request mcp.ListToolsRequest, ) (*mcp.ListToolsResult, *requestError) { s.toolsMu.RLock() tools := make([]mcp.Tool, 0, len(s.tools)) // ← Local to THIS request // Build tools slice... for _, name := range toolNames { tools = append(tools, s.tools[name].Tool) // Struct copy } s.toolsMu.RUnlock() // Apply pagination (returns subslice sharing backing array) toolsToReturn, nextCursor, err := listByPagination(ctx, s, request.Params.Cursor, tools) result := mcp.ListToolsResult{ Tools: toolsToReturn, // ← Shares backing array with local 'tools' } return &result, nil }Why it's safe:
- The local tools variable exists only within this specific request's stack frame
- result.Tools shares the backing array with this local variable
- No other goroutine has a reference to this local backing array
- The mutex (toolsMu) is released before the hook runs, but that only protected the global s.tools map, not this local slice
The actual race (now fixed) was in nested structures:
- Tool structs contain InputSchema.Properties (map) and InputSchema.Required (slice)
- These nested references were shared across all requests (line 1179 copies the struct, not deep copies)
- Without cloning them (lines 55, 64), concurrent writes to the same map/slice caused "concurrent map writes" panics
Verification: The test TestIntentCaptureConcurrentListTools passes with -race detector, confirming no race on line 39.
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.
Ok, LGTM.
928df14 to
7288e17
Compare
|
View all feedbacks in Devflow UI.
This pull request is not mergeable according to GitHub. Common reasons include pending required checks, missing approvals, or merge conflicts — but it could also be blocked by other repository rules or settings.
devflow unqueued this merge request: It did not become mergeable within the expected time |
|
View all feedbacks in Devflow UI.
This pull request is not mergeable according to GitHub. Common reasons include pending required checks, missing approvals, or merge conflicts — but it could also be blocked by other repository rules or settings.
devflow unqueued this merge request: It did not become mergeable within the expected time |

Fixes the cause of https://app.datadoghq.com/incidents/48132
The Tools are shared between requests, and the intent capture logic caused a concurrent writes error, crashing the server. The change resolves that issue.