docs(client): improve server reliability and error handling #560
Conversation
- Add a health check step using Ping to verify server availability - Change error handling for listing tools and resources to terminate the program on failure Signed-off-by: Bo-Yi Wu <[email protected]>
WalkthroughAdds a post-initialization health check calling Ping(ctx) in the sample client and changes ListTools and ListResources error handling to use log.Fatalf on failure; success outputs remain and no exported APIs were modified. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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)
examples/simple_client/main.go (1)
151-156: Refactor error handling for client cleanup and drop redundant pagination note
The client’s
ListResourcesmethod already aggregates pages internally (seeclient/client.go:243–265), so no manual pagination loop is needed.Introduce a
fatalfhelper inexamples/simple_client/main.goto close the client before exiting:// Add just after imports: func fatalf(c *client.Client, format string, args ...interface{}) { c.Close() log.Fatalf(format, args...) }Replace the
log.Fatalfcall on ListResources error (line 151) withfatalf(c, ...):- if err != nil { - log.Fatalf("Failed to list resources: %v", err) - } + if err != nil { + fatalf(c, "Failed to list resources: %v", err) + }Optionally, add
defer c.Close()immediately after initializingc(in the stdio/http branches) to ensure cleanup on normal exit paths.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
examples/simple_client/main.go(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-30T07:13:17.052Z
Learnt from: ezynda3
PR: mark3labs/mcp-go#461
File: server/sampling.go:22-26
Timestamp: 2025-06-30T07:13:17.052Z
Learning: In the mark3labs/mcp-go project, the MCPServer.capabilities field is a struct value (serverCapabilities), not a pointer, so it cannot be nil and doesn't require nil checking. Only pointer fields within the capabilities struct should be checked for nil.
Applied to files:
examples/simple_client/main.go
🧬 Code graph analysis (1)
examples/simple_client/main.go (1)
mcp/tools.go (1)
ListToolsRequest(18-21)
🔇 Additional comments (1)
examples/simple_client/main.go (1)
137-142: I’ve requested the example’s main.go context and checked for any existingfatalfhelper. Once we have that, I can confirm how to best implement cleanup on errors and finalize the review.
examples/simple_client/main.go
Outdated
| // Perform health check using ping | ||
| fmt.Println("Performing health check...") | ||
| if err := c.Ping(ctx); err != nil { | ||
| log.Fatalf("❌ Health check failed: %v", err) | ||
| } | ||
| fmt.Println("✅ Server is alive and responding") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Graceful teardown before fatal exit to avoid leaking the server process (stdio) or sockets (HTTP).
log.Fatalf calls os.Exit(1), so defers won’t run and c.Close() won’t execute. If the stdio client has already started the server process, this can orphan it; for HTTP it can leave connections unclosed. Close the client explicitly before exiting.
Apply this diff in place:
- if err := c.Ping(ctx); err != nil {
- log.Fatalf("❌ Health check failed: %v", err)
- }
+ if err := c.Ping(ctx); err != nil {
+ fatalf(c, "❌ Health check failed: %v", err)
+ }Additionally (optional): use a short ping timeout and a couple of retries with backoff to reduce flakiness when a server is still warming up.
Supporting helper to add elsewhere in this file:
// fatalf closes the client (best-effort) before exiting fatally.
func fatalf(c *client.Client, format string, args ...any) {
if c != nil {
_ = c.Close()
}
log.Fatalf(format, args...)
}🤖 Prompt for AI Agents
In examples/simple_client/main.go around lines 124 to 130, the health-check uses
log.Fatalf which exits immediately and prevents defers (including c.Close())
from running; replace the fatal call with a helper that closes the client before
exiting (add a small fatalf(c *client.Client, format string, args ...any) helper
that best-effort calls c.Close() then calls log.Fatalf), update the Ping error
path to call fatalf(c, "...", err) instead of log.Fatalf, and optionally
implement a short context timeout and a couple of retry attempts with
exponential backoff around c.Ping to reduce flakiness while the server is
warming up.
- Remove emojis from health check log and success messages for a more neutral output Signed-off-by: appleboy <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
examples/simple_client/main.go (1)
124-130: Close client before fatal; add a short ping timeout (optional retry).Exiting via log.Fatalf skips cleanup (c.Close), potentially orphaning the server/transport. Also, bound the ping with a short timeout to avoid consuming the whole 30s root context.
Apply within this hunk:
- // Perform health check using ping - fmt.Println("Performing health check...") - if err := c.Ping(ctx); err != nil { - log.Fatalf("Health check failed: %v", err) - } - fmt.Println("Server is alive and responding") + // Perform health check using ping + fmt.Println("Performing health check...") + pingCtx, cancelPing := context.WithTimeout(ctx, 5*time.Second) + defer cancelPing() + if err := c.Ping(pingCtx); err != nil { + fatalf(c, "Health check failed: %v", err) + } + fmt.Println("Server is alive and responding")Add this helper elsewhere in the file:
// fatalf closes the client (best-effort) before fatally exiting. func fatalf(c *client.Client, format string, args ...any) { if c != nil { _ = c.Close() } log.Fatalf(format, args...) }Run to find remaining log.Fatalf sites to migrate to fatalf:
#!/bin/bash rg -n 'log\.Fatalf\(' examples/simple_client/main.go
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
examples/simple_client/main.go(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-30T07:13:17.052Z
Learnt from: ezynda3
PR: mark3labs/mcp-go#461
File: server/sampling.go:22-26
Timestamp: 2025-06-30T07:13:17.052Z
Learning: In the mark3labs/mcp-go project, the MCPServer.capabilities field is a struct value (serverCapabilities), not a pointer, so it cannot be nil and doesn't require nil checking. Only pointer fields within the capabilities struct should be checked for nil.
Applied to files:
examples/simple_client/main.go
🧬 Code graph analysis (1)
examples/simple_client/main.go (1)
mcp/tools.go (1)
ListToolsRequest(18-21)
| log.Fatalf("Failed to list tools: %v", err) | ||
| } | ||
| fmt.Printf("Server has %d tools available\n", len(toolsResult.Tools)) | ||
| for i, tool := range toolsResult.Tools { | ||
| fmt.Printf(" %d. %s - %s\n", i+1, tool.Name, tool.Description) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard ListTools result against nil and ensure cleanup on fatal.
If err is nil but toolsResult is unexpectedly nil, len(toolsResult.Tools) will panic. Also replace log.Fatalf with fatalf to close the client.
- log.Fatalf("Failed to list tools: %v", err)
+ fatalf(c, "Failed to list tools: %v", err)
}
- fmt.Printf("Server has %d tools available\n", len(toolsResult.Tools))
- for i, tool := range toolsResult.Tools {
+ if toolsResult == nil {
+ fatalf(c, "ListTools returned nil result without error")
+ }
+ fmt.Printf("Server has %d tools available\n", len(toolsResult.Tools))
+ for i, tool := range toolsResult.Tools {
fmt.Printf(" %d. %s - %s\n", i+1, tool.Name, tool.Description)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| log.Fatalf("Failed to list tools: %v", err) | |
| } | |
| fmt.Printf("Server has %d tools available\n", len(toolsResult.Tools)) | |
| for i, tool := range toolsResult.Tools { | |
| fmt.Printf(" %d. %s - %s\n", i+1, tool.Name, tool.Description) | |
| } | |
| fatalf(c, "Failed to list tools: %v", err) | |
| } | |
| if toolsResult == nil { | |
| fatalf(c, "ListTools returned nil result without error") | |
| } | |
| fmt.Printf("Server has %d tools available\n", len(toolsResult.Tools)) | |
| for i, tool := range toolsResult.Tools { | |
| fmt.Printf(" %d. %s - %s\n", i+1, tool.Name, tool.Description) | |
| } |
🤖 Prompt for AI Agents
In examples/simple_client/main.go around lines 137 to 142, guard against a nil
toolsResult (and nil Tools slice) before calling len to avoid a panic, and
replace log.Fatalf with the package's fatalf helper so the client is closed on
error; specifically, after the ListTools call, check if toolsResult == nil or
toolsResult.Tools == nil and call fatalf with an appropriate message when that
happens, and use fatalf for the existing error branch instead of log.Fatalf so
cleanup runs.
| log.Fatalf("Failed to list resources: %v", err) | ||
| } | ||
| fmt.Printf("Server has %d resources available\n", len(resourcesResult.Resources)) | ||
| for i, resource := range resourcesResult.Resources { | ||
| fmt.Printf(" %d. %s - %s\n", i+1, resource.URI, resource.Name) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard ListResources result against nil and ensure cleanup on fatal.
Same panic risk and cleanup concern as tools; harden the branch.
- log.Fatalf("Failed to list resources: %v", err)
+ fatalf(c, "Failed to list resources: %v", err)
}
- fmt.Printf("Server has %d resources available\n", len(resourcesResult.Resources))
- for i, resource := range resourcesResult.Resources {
+ if resourcesResult == nil {
+ fatalf(c, "ListResources returned nil result without error")
+ }
+ fmt.Printf("Server has %d resources available\n", len(resourcesResult.Resources))
+ for i, resource := range resourcesResult.Resources {
fmt.Printf(" %d. %s - %s\n", i+1, resource.URI, resource.Name)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| log.Fatalf("Failed to list resources: %v", err) | |
| } | |
| fmt.Printf("Server has %d resources available\n", len(resourcesResult.Resources)) | |
| for i, resource := range resourcesResult.Resources { | |
| fmt.Printf(" %d. %s - %s\n", i+1, resource.URI, resource.Name) | |
| } | |
| // after calling ListResources: | |
| resourcesResult, err := c.ListResources(ctx) | |
| if err != nil { | |
| fatalf(c, "Failed to list resources: %v", err) | |
| } | |
| if resourcesResult == nil { | |
| fatalf(c, "ListResources returned nil result without error") | |
| } | |
| fmt.Printf("Server has %d resources available\n", len(resourcesResult.Resources)) | |
| for i, resource := range resourcesResult.Resources { | |
| fmt.Printf(" %d. %s - %s\n", i+1, resource.URI, resource.Name) | |
| } |
🤖 Prompt for AI Agents
In examples/simple_client/main.go around lines 151 to 156, guard against a nil
resourcesResult or nil resources slice before accessing len or iterating, and
ensure any required cleanup runs before exiting on error: check if err != nil
then perform cleanup (or call the client.Close/cleanup function) and then exit;
also after a successful call verify resourcesResult != nil and
resourcesResult.Resources != nil before using len/resourcesResult.Resources in
the loop, returning or logging a clear message if nil to avoid panics.
Description
Type of Change
Checklist
MCP Spec Compliance
Additional Information
Summary by CodeRabbit
New Features
Refactor