Skip to content

docs(client): improve server reliability and error handling #560

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Sep 2, 2025
Merged

docs(client): improve server reliability and error handling #560

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ddd4db5
docs(client): improve server reliability and error handling
appleboy Aug 25, 2025
ffc9f87
style: remove emojis for neutral health check log output
appleboy Aug 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 17 additions & 12 deletions examples/simple_client/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,18 +121,24 @@ func main() {
serverInfo.ServerInfo.Version)
fmt.Printf("Server capabilities: %+v\n", serverInfo.Capabilities)

// 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")

Copy link
Contributor

@coderabbitai coderabbitai bot Aug 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

// List available tools if the server supports them
if serverInfo.Capabilities.Tools != nil {
fmt.Println("Fetching available tools...")
toolsRequest := mcp.ListToolsRequest{}
toolsResult, err := c.ListTools(ctx, toolsRequest)
if err != nil {
log.Printf("Failed to list tools: %v", err)
} else {
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)
}
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)
}
Comment on lines +137 to 142
Copy link
Contributor

@coderabbitai coderabbitai bot Aug 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

⚠️ Potential issue

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.

Suggested change
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.

}

Expand All @@ -142,12 +148,11 @@ func main() {
resourcesRequest := mcp.ListResourcesRequest{}
resourcesResult, err := c.ListResources(ctx, resourcesRequest)
if err != nil {
log.Printf("Failed to list resources: %v", err)
} else {
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)
}
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)
}
Comment on lines +151 to 156
Copy link
Contributor

@coderabbitai coderabbitai bot Aug 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

⚠️ Potential issue

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.

Suggested change
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.

}

Expand Down