Skip to content

Commit 2753ac5

Browse files
committed
Refactor OAuth handling and error management in client connection
- Updated ShouldUseOAuth to prioritize header authentication before falling back to OAuth. - Enhanced getServerToolCount with shorter timeout and improved error classification for UI status updates. - Implemented detailed error handling in connect methods to provide clearer context for authentication failures. - Introduced helper functions for error classification to streamline connection logic and improve maintainability.
1 parent 5e7ca92 commit 2753ac5

4 files changed

Lines changed: 495 additions & 657 deletions

File tree

internal/oauth/config.go

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -305,7 +305,7 @@ func parseBaseURL(fullURL string) (string, error) {
305305
}
306306

307307
// ShouldUseOAuth determines if OAuth should be attempted for a given server
308-
// This is now based on protocol type only - OAuth detection happens when server returns 401
308+
// Headers are tried first if configured, then OAuth as fallback on auth errors
309309
func ShouldUseOAuth(serverConfig *config.ServerConfig) bool {
310310
logger := zap.L().Named("oauth")
311311

@@ -321,9 +321,16 @@ func ShouldUseOAuth(serverConfig *config.ServerConfig) bool {
321321
return false
322322
}
323323

324-
// For HTTP/SSE servers, we should always try OAuth-enabled clients
325-
// The mcp-go library will handle the OAuth flow only if the server requires it (returns 401)
326-
logger.Debug("OAuth-enabled client will be used",
324+
// If headers are configured, try headers first, not OAuth
325+
if len(serverConfig.Headers) > 0 {
326+
logger.Debug("Headers configured - will try headers first, OAuth as fallback if needed",
327+
zap.String("server", serverConfig.Name),
328+
zap.Int("header_count", len(serverConfig.Headers)))
329+
return false
330+
}
331+
332+
// For HTTP/SSE servers without headers, try OAuth-enabled clients
333+
logger.Debug("No headers configured - OAuth-enabled client will be used",
327334
zap.String("server", serverConfig.Name),
328335
zap.String("protocol", serverConfig.Protocol))
329336

internal/server/server.go

Lines changed: 39 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -761,25 +761,60 @@ func (s *Server) QuarantineServer(serverName string, quarantined bool) error {
761761
}
762762

763763
// getServerToolCount returns the number of tools for a specific server
764+
// Uses shorter timeout and better error handling for UI status display
764765
func (s *Server) getServerToolCount(serverID string) int {
765766
client, exists := s.upstreamManager.GetClient(serverID)
766767
if !exists || !client.IsConnected() {
767768
return 0
768769
}
769770

770-
ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
771+
// Use shorter timeout for UI status updates (5 seconds instead of 30)
772+
// This reduces waiting time for unresponsive servers
773+
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
771774
defer cancel()
775+
772776
tools, err := client.ListTools(ctx)
773777
if err != nil {
774-
s.logger.Warn("Failed to get tool count for server",
775-
zap.String("server_id", serverID),
776-
zap.Error(err))
778+
// Classify errors to reduce noise from expected failures
779+
if isTimeoutError(err) {
780+
// Timeout errors are common for servers that don't support tool listing
781+
// Log at debug level to reduce noise
782+
s.logger.Debug("Tool listing timeout for server (server may not support tools)",
783+
zap.String("server_id", serverID),
784+
zap.String("error_type", "timeout"))
785+
} else if isConnectionError(err) {
786+
// Connection errors suggest the server is actually disconnected
787+
s.logger.Debug("Connection error during tool listing",
788+
zap.String("server_id", serverID),
789+
zap.String("error_type", "connection"))
790+
} else {
791+
// Other errors might be more significant
792+
s.logger.Warn("Failed to get tool count for server",
793+
zap.String("server_id", serverID),
794+
zap.Error(err))
795+
}
777796
return 0
778797
}
779798

780799
return len(tools)
781800
}
782801

802+
// Helper functions for error classification
803+
func isTimeoutError(err error) bool {
804+
errStr := err.Error()
805+
return strings.Contains(errStr, "timeout") ||
806+
strings.Contains(errStr, "deadline exceeded") ||
807+
strings.Contains(errStr, "context canceled")
808+
}
809+
810+
func isConnectionError(err error) bool {
811+
errStr := err.Error()
812+
return strings.Contains(errStr, "connection refused") ||
813+
strings.Contains(errStr, "no such host") ||
814+
strings.Contains(errStr, "connection reset") ||
815+
strings.Contains(errStr, "broken pipe")
816+
}
817+
783818
// StartServer starts the server if it's not already running
784819
func (s *Server) StartServer(ctx context.Context) error {
785820
s.mu.Lock()

0 commit comments

Comments
 (0)