fix(server): recover panics in executeRegularToolAsTask (hybrid task mode) - #939
Conversation
|
Connected to Huly®: MCP_G-503 |
WalkthroughAdds panic recovery to asynchronous regular tool task execution so panicking handlers complete their tasks with failed status, plus a test that verifies completion and preserved panic details. ChangesRegular tool task recovery
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ 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: 1
🧹 Nitpick comments (1)
server/regular_tool_as_task_panic_test.go (1)
22-41: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExercise the real hybrid dispatch path.
This test calls
executeRegularToolAsTaskdirectly and leavesTool.Executionunset, so it bypasseshandleTaskAugmentedToolCalland does not prove that aTaskSupportOptionalregular tool is routed into this recovery path. Register the tool with optional task support and invoke the production entrypoint.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/regular_tool_as_task_panic_test.go` around lines 22 - 41, Update the panic test to set the tool’s Execution to TaskSupportOptional, register the tool with the server, and invoke handleTaskAugmentedToolCall instead of executeRegularToolAsTask. Preserve the existing task setup and assertions while exercising the production hybrid dispatch path for the registered regular tool.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/server.go`:
- Around line 2225-2228: Sanitize panic details in the task error contract: in
server/server.go lines 2225-2228, retain detailed panic diagnostics in
server-side logging or recovery handling but pass a generic client-safe error to
completeTask. Update server/regular_tool_as_task_panic_test.go lines 52-54 to
assert the generic sanitized status message instead of raw panic text.
---
Nitpick comments:
In `@server/regular_tool_as_task_panic_test.go`:
- Around line 22-41: Update the panic test to set the tool’s Execution to
TaskSupportOptional, register the tool with the server, and invoke
handleTaskAugmentedToolCall instead of executeRegularToolAsTask. Preserve the
existing task setup and assertions while exercising the production hybrid
dispatch path for the registered regular tool.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e12abc4e-2222-4300-b54e-da0d73cf092a
📒 Files selected for processing (2)
server/regular_tool_as_task_panic_test.goserver/server.go
| defer func() { | ||
| if r := recover(); r != nil { | ||
| s.completeTask(entry, nil, fmt.Errorf("panic in task tool handler %s: %v", request.Params.Name, r)) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Sanitize panic details across the task error contract.
Raw panic values can leak sensitive handler or implementation details through task status notifications.
server/server.go#L2225-L2228: record detailed panic diagnostics server-side, but complete the task with a generic client-safe error.server/regular_tool_as_task_panic_test.go#L52-L54: assert the sanitized status message rather than the raw panic text.
📍 Affects 2 files
server/server.go#L2225-L2228(this comment)server/regular_tool_as_task_panic_test.go#L52-L54
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/server.go` around lines 2225 - 2228, Sanitize panic details in the
task error contract: in server/server.go lines 2225-2228, retain detailed panic
diagnostics in server-side logging or recovery handling but pass a generic
client-safe error to completeTask. Update
server/regular_tool_as_task_panic_test.go lines 52-54 to assert the generic
sanitized status message instead of raw panic text.
|
Good catch. I intentionally mirrored the exact pattern from #880 ( If sanitizing panic messages across the task contract is preferred, I think that's best handled as a separate follow-up covering both paths together, rather than creating an inconsistency here. Happy to open that if there's interest! |
- Promote deepset-ai/haystack#12217 from open to merged - Add mark3labs/mcp-go#939 to open - Add 567-labs/instructor#2476 to closed Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Summary
Adds panic recovery to
executeRegularToolAsTask, the goroutine used when a regular tool withTaskSupportOptionalis invoked asynchronously (called with task params) in hybrid mode.Problem
#880 added panic recovery to
executeTaskToolafter a report that an unrecovered panic in that goroutine would crash the whole server. That fix didn't cover the sibling function,executeRegularToolAsTask, which still runs without a recover guard.Reproduced directly: a panicking handler run through this path propagates the panic uncaught out of the goroutine, which in Go terminates the entire process -- not just the failing request. Any tool registered with
TaskSupportOptionalthat panics when called asynchronously currently takes down the server for every connected client.Fix
Mirrors #880's exact pattern -- a
defer/recover()that completes the task asFailedwith the panic message viacompleteTask, instead of letting the panic escape.Testing
TestExecuteRegularToolAsTask_PanicRecovery, mirroring fix: add panic recovery to task goroutines, fix cleanup goroutine leak #880's ownTestExecuteTaskTool_PanicRecovery.main(unrecovered panic) and passes cleanly with this change.go build,go vet,go test ./...,go test -race ./server/...): all green, zero regressions.Note on related open PRs
#900 and #889 both address a different bug in this same function (context cancellation cascading from the HTTP request lifecycle, #897) via different mechanisms. Neither touches panic recovery, and neither conflicts with this change at the line level -- this PR only adds a
deferblock at the top of the function body.Summary by CodeRabbit