From 1e812a610c9865739e6cc66a8d92b42b30fb65cd Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 3 Apr 2026 00:02:52 +0000 Subject: [PATCH 1/5] Initial plan From d1d8095745d22990e60c48ae1bec97fc297534c1 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 3 Apr 2026 00:31:52 +0000 Subject: [PATCH 2/5] fix: Add HTTP keepalive to prevent safeoutputs session expiry and extend connection pool idle timeout Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/e656bbfd-cb96-4f37-b453-1acce2b238df Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> --- internal/launcher/connection_pool.go | 5 +++- internal/mcp/connection.go | 8 ++++-- internal/mcp/connection_test.go | 43 +++++++++++++++++++++++++--- internal/mcp/http_transport.go | 28 ++++++++++++------ 4 files changed, 68 insertions(+), 16 deletions(-) diff --git a/internal/launcher/connection_pool.go b/internal/launcher/connection_pool.go index 629b938f..78475f1a 100644 --- a/internal/launcher/connection_pool.go +++ b/internal/launcher/connection_pool.go @@ -39,7 +39,10 @@ const ( // Default configuration values const ( - DefaultIdleTimeout = 30 * time.Minute + // DefaultIdleTimeout is the maximum duration a connection can remain unused before being + // removed from the pool. Set to 6 hours to accommodate long-running workflow tasks + // (e.g. ML training, large builds) that may not make MCP calls for extended periods. + DefaultIdleTimeout = 6 * time.Hour DefaultCleanupInterval = 5 * time.Minute DefaultMaxErrorCount = 10 ) diff --git a/internal/mcp/connection.go b/internal/mcp/connection.go index 0c398ede..8f9b8c87 100644 --- a/internal/mcp/connection.go +++ b/internal/mcp/connection.go @@ -70,6 +70,7 @@ type Connection struct { httpClient *http.Client httpSessionID string // Session ID returned by the HTTP backend httpTransportType HTTPTransportType // Type of HTTP transport in use + keepAliveInterval time.Duration // Keepalive interval for SDK transports (0 = disabled) // sessionMu protects the mutable session fields: httpSessionID, session, and client. // Always use getHTTPSessionID() or getSDKSession() to read these fields; the // reconnect functions (reconnectPlainJSON, reconnectSDKTransport) hold the full Lock. @@ -98,8 +99,8 @@ func NewConnection(ctx context.Context, serverID, command string, args []string, logger.LogInfo("backend", "Creating new MCP backend connection, command=%s, args=%v", command, sanitize.SanitizeArgs(args)) ctx, cancel := context.WithCancel(ctx) - // Create MCP client with logger - client := newMCPClient(logConn) + // Create MCP client with logger (no keepalive for stdio – the process lifespan manages the session) + client := newMCPClient(logConn, 0) // Expand Docker -e flags that reference environment variables // Docker's `-e VAR_NAME` expects VAR_NAME to be in the environment @@ -326,7 +327,8 @@ func (c *Connection) reconnectSDKTransport() error { headerClient := buildHTTPClientWithHeaders(c.httpClient, c.headers) // Build the appropriate transport. - client := newMCPClient(logConn) + // Re-use the same keepAliveInterval so the reconnected session also sends periodic pings. + client := newMCPClient(logConn, c.keepAliveInterval) var transport sdk.Transport switch c.httpTransportType { case HTTPTransportStreamable: diff --git a/internal/mcp/connection_test.go b/internal/mcp/connection_test.go index 5da70789..2378312e 100644 --- a/internal/mcp/connection_test.go +++ b/internal/mcp/connection_test.go @@ -9,6 +9,7 @@ import ( "os" "strings" "testing" + "time" "github.com/github/gh-aw-mcpg/internal/config" "github.com/github/gh-aw-mcpg/internal/difc" @@ -507,17 +508,51 @@ func stringContains(s, substr string) bool { // TestNewMCPClient tests the newMCPClient helper function func TestNewMCPClient(t *testing.T) { - client := newMCPClient(nil) + client := newMCPClient(nil, 0) require.NotNil(t, client, "newMCPClient should return a non-nil client") } // TestNewMCPClientWithLogger tests that newMCPClient accepts a logger func TestNewMCPClientWithLogger(t *testing.T) { log := logger.New("test:client") - client := newMCPClient(log) + client := newMCPClient(log, 0) require.NotNil(t, client, "newMCPClient should return a non-nil client with logger") } +// TestNewMCPClientWithKeepalive tests that newMCPClient accepts a keepalive interval +func TestNewMCPClientWithKeepalive(t *testing.T) { + client := newMCPClient(nil, DefaultHTTPKeepaliveInterval) + require.NotNil(t, client, "newMCPClient should return a non-nil client with keepalive") +} + +// TestDefaultHTTPKeepaliveInterval verifies the keepalive constant is less than a typical +// backend session timeout (30 minutes) to prevent session expiry during long agent runs. +func TestDefaultHTTPKeepaliveInterval(t *testing.T) { + const typicalBackendTimeout = 30 * time.Minute + assert.Less(t, DefaultHTTPKeepaliveInterval, typicalBackendTimeout, + "DefaultHTTPKeepaliveInterval must be less than the typical backend session timeout to prevent expiry") + assert.Greater(t, DefaultHTTPKeepaliveInterval, time.Duration(0), + "DefaultHTTPKeepaliveInterval must be positive") +} + +// TestNewHTTPConnectionStoresKeepalive verifies that the keepalive interval is stored on +// the connection struct so that reconnectSDKTransport can recreate the session with the same setting. +func TestNewHTTPConnectionStoresKeepalive(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + client := newMCPClient(nil, DefaultHTTPKeepaliveInterval) + url := "http://example.com/mcp" + headers := map[string]string{} + httpClient := &http.Client{} + + conn := newHTTPConnection(ctx, cancel, client, nil, url, headers, httpClient, HTTPTransportStreamable, "test-server", DefaultHTTPKeepaliveInterval) + + require.NotNil(t, conn) + assert.Equal(t, DefaultHTTPKeepaliveInterval, conn.keepAliveInterval, + "keepAliveInterval should be stored on the connection for use during reconnection") +} + // TestSetupHTTPRequest tests the setupHTTPRequest helper function func TestSetupHTTPRequest(t *testing.T) { tests := []struct { @@ -593,12 +628,12 @@ func TestNewHTTPConnection(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) defer cancel() - client := newMCPClient(nil) + client := newMCPClient(nil, 0) url := "http://example.com/mcp" headers := map[string]string{"Authorization": "test"} httpClient := &http.Client{} - conn := newHTTPConnection(ctx, cancel, client, nil, url, headers, httpClient, HTTPTransportStreamable, "test-server") + conn := newHTTPConnection(ctx, cancel, client, nil, url, headers, httpClient, HTTPTransportStreamable, "test-server", 0) require.NotNil(t, conn, "Connection should not be nil") assert.Equal(t, client, conn.client, "Client should match") diff --git a/internal/mcp/http_transport.go b/internal/mcp/http_transport.go index 38a2638f..0f188e92 100644 --- a/internal/mcp/http_transport.go +++ b/internal/mcp/http_transport.go @@ -36,6 +36,11 @@ const ( // MCPProtocolVersion is the MCP protocol version used in initialization requests. const MCPProtocolVersion = "2025-11-25" +// DefaultHTTPKeepaliveInterval is the default interval for sending keepalive pings to HTTP backends. +// This should be less than the backend's session idle timeout (typically 30 minutes for safeoutputs). +// A value of 25 minutes gives a 5-minute buffer before the session would expire. +const DefaultHTTPKeepaliveInterval = 25 * time.Minute + // requestIDCounter is used to generate unique request IDs for HTTP requests var requestIDCounter uint64 @@ -167,7 +172,8 @@ func parseJSONRPCResponseWithSSE(body []byte, statusCode int, contextDesc string // newMCPClient creates a new MCP SDK client with standard implementation details // Pass nil for logger parameter to disable SDK logging (for tests) -func newMCPClient(log *logger.Logger) *sdk.Client { +// Pass keepAlive > 0 to enable periodic ping keepalives (recommended for HTTP backends) +func newMCPClient(log *logger.Logger, keepAlive time.Duration) *sdk.Client { var slogLogger *slog.Logger if log != nil { slogLogger = logger.NewSlogLoggerWithHandler(log) @@ -176,18 +182,21 @@ func newMCPClient(log *logger.Logger) *sdk.Client { Name: "awmg", Version: version.Get(), }, &sdk.ClientOptions{ - Logger: slogLogger, + Logger: slogLogger, + KeepAlive: keepAlive, }) } -// newHTTPConnection creates a new HTTP Connection struct with common fields -func newHTTPConnection(ctx context.Context, cancel context.CancelFunc, client *sdk.Client, session *sdk.ClientSession, url string, headers map[string]string, httpClient *http.Client, transportType HTTPTransportType, serverID string) *Connection { +// newHTTPConnection creates a new HTTP Connection struct with common fields. +// keepAlive is passed through to store on the connection so that reconnectSDKTransport +// can re-create the SDK client with the same keepalive setting. +func newHTTPConnection(ctx context.Context, cancel context.CancelFunc, client *sdk.Client, session *sdk.ClientSession, url string, headers map[string]string, httpClient *http.Client, transportType HTTPTransportType, serverID string, keepAlive time.Duration) *Connection { // Extract session ID from SDK session if available var sessionID string if session != nil { sessionID = session.ID() } - logHTTP.Printf("Creating HTTP connection: serverID=%s, url=%s, transport=%s, headers=%d, sessionID=%s", serverID, url, transportType, len(headers), sessionID) + logHTTP.Printf("Creating HTTP connection: serverID=%s, url=%s, transport=%s, headers=%d, sessionID=%s, keepAlive=%v", serverID, url, transportType, len(headers), sessionID, keepAlive) return &Connection{ client: client, session: session, @@ -200,6 +209,7 @@ func newHTTPConnection(ctx context.Context, cancel context.CancelFunc, client *s httpClient: httpClient, httpTransportType: transportType, httpSessionID: sessionID, + keepAliveInterval: keepAlive, } } @@ -394,8 +404,10 @@ func trySDKTransport( transportName string, createTransport transportConnector, ) (*Connection, error) { - // Create MCP client with logger - client := newMCPClient(logConn) + // Create MCP client with logger and keepalive enabled. + // DefaultHTTPKeepaliveInterval sends periodic pings to prevent session expiry + // on backends (e.g. safeoutputs) that have a 30-minute idle timeout. + client := newMCPClient(logConn, DefaultHTTPKeepaliveInterval) // Create transport using the provided connector transport := createTransport(url, httpClient) @@ -410,7 +422,7 @@ func trySDKTransport( return nil, fmt.Errorf("%s transport connect failed: %w", transportName, err) } - conn := newHTTPConnection(ctx, cancel, client, session, url, headers, httpClient, transportType, serverID) + conn := newHTTPConnection(ctx, cancel, client, session, url, headers, httpClient, transportType, serverID, DefaultHTTPKeepaliveInterval) logger.LogInfo("backend", "%s transport connected successfully", transportName) logConn.Printf("Connected with %s transport", transportName) From 66d91466ca1390c74a3da81e182fae1fedf78904 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 3 Apr 2026 00:33:30 +0000 Subject: [PATCH 3/5] docs: Improve connection pool idle timeout comments to clarify STDIO vs HTTP session management Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/e656bbfd-cb96-4f37-b453-1acce2b238df Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> --- internal/launcher/connection_pool.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/internal/launcher/connection_pool.go b/internal/launcher/connection_pool.go index 78475f1a..6bcf9a0e 100644 --- a/internal/launcher/connection_pool.go +++ b/internal/launcher/connection_pool.go @@ -39,9 +39,12 @@ const ( // Default configuration values const ( - // DefaultIdleTimeout is the maximum duration a connection can remain unused before being - // removed from the pool. Set to 6 hours to accommodate long-running workflow tasks - // (e.g. ML training, large builds) that may not make MCP calls for extended periods. + // DefaultIdleTimeout is the maximum duration a STDIO-backend connection can remain unused + // before being removed from the session pool. Set to 6 hours to accommodate long-running + // workflow tasks (e.g. ML training, large builds) that may not make MCP calls for extended + // periods. Note: this is distinct from HTTP backend keepalive (DefaultHTTPKeepaliveInterval) + // which keeps the remote session alive on the HTTP server side; STDIO connections run as local + // child processes whose sessions are bounded only by this pool eviction window. DefaultIdleTimeout = 6 * time.Hour DefaultCleanupInterval = 5 * time.Minute DefaultMaxErrorCount = 10 From 97149f051b8a2dda15de5c80994f3403f677fefa Mon Sep 17 00:00:00 2001 From: Landon Cox Date: Thu, 2 Apr 2026 17:48:26 -0700 Subject: [PATCH 4/5] feat: make HTTP keepalive interval configurable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add keepalive_interval field to GatewayConfig (seconds). Default 1500 (25 min) preserves current behavior. Set to -1 to disable keepalive pings entirely when higher-level timeouts manage session lifecycle. Thread the configurable duration through NewHTTPConnection → trySDKTransport → newMCPClient instead of hardcoding DefaultHTTPKeepaliveInterval. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- internal/config/config_core.go | 27 +++++++++++++++++--- internal/launcher/launcher.go | 2 +- internal/mcp/connection.go | 6 ++--- internal/mcp/connection_arguments_test.go | 4 +-- internal/mcp/connection_stderr_test.go | 2 +- internal/mcp/connection_test.go | 12 ++++----- internal/mcp/http_connection_test.go | 28 ++++++++++----------- internal/mcp/http_error_propagation_test.go | 8 +++--- internal/mcp/http_transport.go | 17 +++++++------ internal/mcp/http_transport_test.go | 10 ++++---- test/integration/http_error_test.go | 22 ++++++++-------- 11 files changed, 81 insertions(+), 57 deletions(-) diff --git a/internal/config/config_core.go b/internal/config/config_core.go index 1aa72d58..7de4c589 100644 --- a/internal/config/config_core.go +++ b/internal/config/config_core.go @@ -29,6 +29,7 @@ import ( "io" "log" "os" + "time" "github.com/BurntSushi/toml" "github.com/github/gh-aw-mcpg/internal/logger" @@ -36,9 +37,10 @@ import ( // Core constants for configuration defaults const ( - DefaultPort = 3000 - DefaultStartupTimeout = 60 // seconds - DefaultToolTimeout = 120 // seconds + DefaultPort = 3000 + DefaultStartupTimeout = 60 // seconds + DefaultToolTimeout = 120 // seconds + DefaultKeepaliveInterval = 1500 // seconds (25 minutes) — keeps HTTP backend sessions alive ) // Config represents the internal gateway configuration. @@ -87,6 +89,13 @@ type GatewayConfig struct { // ToolTimeout is the maximum time (seconds) to wait for tool execution ToolTimeout int `toml:"tool_timeout" json:"tool_timeout,omitempty"` + // KeepaliveInterval is the interval (seconds) for sending keepalive pings to HTTP + // backends. This prevents long-running sessions from being expired by the remote + // server's idle timeout (typically 30 minutes). Set to -1 to disable keepalive + // pings entirely (useful when higher-level timeouts manage session lifecycle). + // Default: 1500 (25 minutes) + KeepaliveInterval int `toml:"keepalive_interval" json:"keepalive_interval,omitempty"` + // PayloadDir is the directory for storing large payloads PayloadDir string `toml:"payload_dir" json:"payload_dir,omitempty"` @@ -110,6 +119,15 @@ type GatewayConfig struct { TrustedBots []string `toml:"trusted_bots" json:"trusted_bots,omitempty"` } +// HTTPKeepaliveInterval returns the keepalive interval as a time.Duration. +// A negative KeepaliveInterval disables keepalive (returns 0). +func (g *GatewayConfig) HTTPKeepaliveInterval() time.Duration { + if g.KeepaliveInterval < 0 { + return 0 + } + return time.Duration(g.KeepaliveInterval) * time.Second +} + // GetAPIKey returns the gateway API key, handling a nil Gateway safely. func (c *Config) GetAPIKey() string { if c.Gateway == nil { @@ -196,6 +214,9 @@ func applyGatewayDefaults(cfg *GatewayConfig) { if cfg.ToolTimeout == 0 { cfg.ToolTimeout = DefaultToolTimeout } + if cfg.KeepaliveInterval == 0 { + cfg.KeepaliveInterval = DefaultKeepaliveInterval + } } // EnsureGatewayDefaults guarantees that cfg.Gateway is non-nil and that all diff --git a/internal/launcher/launcher.go b/internal/launcher/launcher.go index 3285fc10..28185205 100644 --- a/internal/launcher/launcher.go +++ b/internal/launcher/launcher.go @@ -139,7 +139,7 @@ func GetOrLaunch(l *Launcher, serverID string) (*mcp.Connection, error) { } // Create an HTTP connection - conn, err := mcp.NewHTTPConnection(l.ctx, serverID, serverCfg.URL, serverCfg.Headers, oidcProvider, oidcAudience) + conn, err := mcp.NewHTTPConnection(l.ctx, serverID, serverCfg.URL, serverCfg.Headers, oidcProvider, oidcAudience, l.config.Gateway.HTTPKeepaliveInterval()) if err != nil { logger.LogErrorWithServer(serverID, "backend", "Failed to create HTTP connection: %s, error=%v", serverID, err) log.Printf("[LAUNCHER] ❌ FAILED to create HTTP connection for '%s'", serverID) diff --git a/internal/mcp/connection.go b/internal/mcp/connection.go index 8f9b8c87..75074188 100644 --- a/internal/mcp/connection.go +++ b/internal/mcp/connection.go @@ -197,7 +197,7 @@ func NewConnection(ctx context.Context, serverID, command string, args []string, // Authorization header from the headers map. // // This ensures compatibility with all types of HTTP MCP servers. -func NewHTTPConnection(ctx context.Context, serverID, url string, headers map[string]string, oidcProvider *oidc.Provider, oidcAudience string) (*Connection, error) { +func NewHTTPConnection(ctx context.Context, serverID, url string, headers map[string]string, oidcProvider *oidc.Provider, oidcAudience string, keepAlive time.Duration) (*Connection, error) { logger.LogInfo("backend", "Creating HTTP MCP connection with transport fallback, url=%s", url) ctx, cancel := context.WithCancel(ctx) @@ -235,7 +235,7 @@ func NewHTTPConnection(ctx context.Context, serverID, url string, headers map[st // Try 1: Streamable HTTP (2025-03-26 spec) logConn.Printf("Attempting streamable HTTP transport for %s", url) - conn, err := tryStreamableHTTPTransport(ctx, cancel, serverID, url, headers, headerClient) + conn, err := tryStreamableHTTPTransport(ctx, cancel, serverID, url, headers, headerClient, keepAlive) if err == nil { logger.LogInfo("backend", "Successfully connected using streamable HTTP transport, url=%s", url) log.Printf("Configured HTTP MCP server with streamable transport: %s", url) @@ -245,7 +245,7 @@ func NewHTTPConnection(ctx context.Context, serverID, url string, headers map[st // Try 2: SSE (2024-11-05 spec) logConn.Printf("Attempting SSE transport for %s", url) - conn, err = trySSETransport(ctx, cancel, serverID, url, headers, headerClient) + conn, err = trySSETransport(ctx, cancel, serverID, url, headers, headerClient, keepAlive) if err == nil { logger.LogWarn("backend", "⚠️ MCP over SSE has been deprecated. Connected using SSE transport for url=%s. Please migrate to streamable HTTP transport (2025-03-26 spec).", url) log.Printf("⚠️ WARNING: MCP over SSE (2024-11-05 spec) has been DEPRECATED") diff --git a/internal/mcp/connection_arguments_test.go b/internal/mcp/connection_arguments_test.go index a2fa4115..b8661b76 100644 --- a/internal/mcp/connection_arguments_test.go +++ b/internal/mcp/connection_arguments_test.go @@ -159,7 +159,7 @@ func TestCallTool_ArgumentsPassed(t *testing.T) { // Create connection conn, err := NewHTTPConnection(context.Background(), "test-server", testServer.URL, map[string]string{ "Authorization": "test-token", - }, nil, "") + }, nil, "", 0) require.NoError(t, err, "Failed to create HTTP connection") defer conn.Close() @@ -224,7 +224,7 @@ func TestCallTool_MissingArguments(t *testing.T) { conn, err := NewHTTPConnection(context.Background(), "test-server", testServer.URL, map[string]string{ "Authorization": "test-token", - }, nil, "") + }, nil, "", 0) require.NoError(t, err) defer conn.Close() diff --git a/internal/mcp/connection_stderr_test.go b/internal/mcp/connection_stderr_test.go index f0c9a241..6b6d65ea 100644 --- a/internal/mcp/connection_stderr_test.go +++ b/internal/mcp/connection_stderr_test.go @@ -37,7 +37,7 @@ func TestConnection_SendRequest(t *testing.T) { conn, err := NewHTTPConnection(context.Background(), "test-server", srv.URL, map[string]string{ "Authorization": "test-token", - }, nil, "") + }, nil, "", 0) require.NoError(t, err) defer conn.Close() diff --git a/internal/mcp/connection_test.go b/internal/mcp/connection_test.go index 2378312e..0d77d203 100644 --- a/internal/mcp/connection_test.go +++ b/internal/mcp/connection_test.go @@ -42,7 +42,7 @@ func TestHTTPRequest_SessionIDHeader(t *testing.T) { // Create an HTTP connection conn, err := NewHTTPConnection(context.Background(), "test-server", testServer.URL, map[string]string{ "Authorization": "test-auth-token", - }, nil, "") + }, nil, "", 0) require.NoError(t, err, "Failed to create HTTP connection") // Create a context with session ID @@ -79,7 +79,7 @@ func TestHTTPRequest_NoSessionID(t *testing.T) { // Create an HTTP connection conn, err := NewHTTPConnection(context.Background(), "test-server", testServer.URL, map[string]string{ "Authorization": "test-auth-token", - }, nil, "") + }, nil, "", 0) require.NoError(t, err, "Failed to create HTTP connection") // Send a request without session ID in context @@ -117,7 +117,7 @@ func TestHTTPRequest_ConfiguredHeaders(t *testing.T) { authToken := "configured-auth-token" conn, err := NewHTTPConnection(context.Background(), "test-server", testServer.URL, map[string]string{ "Authorization": authToken, - }, nil, "") + }, nil, "", 0) require.NoError(t, err, "Failed to create HTTP connection") // Create a context with session ID @@ -376,7 +376,7 @@ func TestHTTPRequest_ErrorResponses(t *testing.T) { // Create connection with custom headers to use plain JSON transport conn, err := NewHTTPConnection(context.Background(), "test-server", testServer.URL, map[string]string{ "Authorization": "test-token", - }, nil, "") + }, nil, "", 0) if err != nil && tt.expectError { // Error during initialization is expected for some error conditions if tt.errorSubstring != "" && !containsSubstring(err.Error(), tt.errorSubstring) { @@ -428,7 +428,7 @@ func TestConnection_IsHTTP(t *testing.T) { "X-Custom": "custom-value", } - conn, err := NewHTTPConnection(context.Background(), "test-server", testServer.URL, headers, nil, "") + conn, err := NewHTTPConnection(context.Background(), "test-server", testServer.URL, headers, nil, "", 0) require.NoError(t, err, "Failed to create HTTP connection") defer conn.Close() @@ -475,7 +475,7 @@ func TestHTTPConnection_InvalidURL(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - _, err := NewHTTPConnection(context.Background(), "test-server", tt.url, tt.headers, nil, "") + _, err := NewHTTPConnection(context.Background(), "test-server", tt.url, tt.headers, nil, "", 0) if tt.expectError { if err == nil { diff --git a/internal/mcp/http_connection_test.go b/internal/mcp/http_connection_test.go index fc59c482..018b6592 100644 --- a/internal/mcp/http_connection_test.go +++ b/internal/mcp/http_connection_test.go @@ -54,7 +54,7 @@ func TestNewHTTPConnection_WithCustomHeaders(t *testing.T) { "X-Custom-Header": "custom-value", } - conn, err := NewHTTPConnection(context.Background(), "test-server", testServer.URL, customHeaders, nil, "") + conn, err := NewHTTPConnection(context.Background(), "test-server", testServer.URL, customHeaders, nil, "", 0) require.NoError(err, "Failed to create HTTP connection with custom headers") require.NotNil(conn, "Connection should not be nil") defer conn.Close() @@ -112,7 +112,7 @@ func TestNewHTTPConnection_WithoutHeaders_FallbackSequence(t *testing.T) { defer testServer.Close() // Create connection without custom headers - streamable transport should succeed first - conn, err := NewHTTPConnection(context.Background(), "test-server", testServer.URL, nil, nil, "") + conn, err := NewHTTPConnection(context.Background(), "test-server", testServer.URL, nil, nil, "", 0) require.NoError(err, "Connection should succeed") require.NotNil(conn) defer conn.Close() @@ -134,7 +134,7 @@ func TestNewHTTPConnection_AllTransportsFail(t *testing.T) { defer testServer.Close() // Try to create connection without custom headers (will try all transports) - conn, err := NewHTTPConnection(context.Background(), "test-server", testServer.URL, nil, nil, "") + conn, err := NewHTTPConnection(context.Background(), "test-server", testServer.URL, nil, nil, "", 0) // Should fail after trying all transports require.Error(err, "Should fail when all transports fail") @@ -161,7 +161,7 @@ func TestNewHTTPConnection_ContextCancellation(t *testing.T) { cancel() // Try to create connection with cancelled context - conn, err := NewHTTPConnection(ctx, "test-server", testServer.URL, map[string]string{"Auth": "token"}, nil, "") + conn, err := NewHTTPConnection(ctx, "test-server", testServer.URL, map[string]string{"Auth": "token"}, nil, "", 0) // Should fail due to context cancellation require.Error(err, "Should fail with cancelled context") @@ -198,7 +198,7 @@ func TestNewHTTPConnection_InvalidURL(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - conn, err := NewHTTPConnection(context.Background(), "test-server", tt.url, tt.headers, nil, "") + conn, err := NewHTTPConnection(context.Background(), "test-server", tt.url, tt.headers, nil, "", 0) if tt.expectError { assert.Error(t, err, "Expected error for invalid URL") @@ -273,7 +273,7 @@ func TestTryPlainJSONTransport_InitializeFailure(t *testing.T) { // Try to create connection conn, err := NewHTTPConnection(context.Background(), "test-server", testServer.URL, map[string]string{ "Authorization": "test-token", - }, nil, "") + }, nil, "", 0) // Should fail with appropriate error require.Error(t, err, "Should fail on initialization error") @@ -306,7 +306,7 @@ data: {"jsonrpc":"2.0","id":1,"result":{"protocolVersion":"2024-11-05","serverIn // Create connection conn, err := NewHTTPConnection(context.Background(), "test-server", testServer.URL, map[string]string{ "Authorization": "test-token", - }, nil, "") + }, nil, "", 0) require.NoError(err, "Should successfully parse SSE-formatted initialize response") require.NotNil(conn) @@ -347,7 +347,7 @@ func TestHTTPConnection_NoSessionIDInResponse(t *testing.T) { // Create connection conn, err := NewHTTPConnection(context.Background(), "test-server", testServer.URL, map[string]string{ "Authorization": "test-token", - }, nil, "") + }, nil, "", 0) require.NoError(err, "Should succeed even without session ID from server") require.NotNil(conn) @@ -393,7 +393,7 @@ func TestNewHTTPConnection_HeadersPropagation(t *testing.T) { "X-Custom-2": "value2", } - conn, err := NewHTTPConnection(context.Background(), "test-server", testServer.URL, customHeaders, nil, "") + conn, err := NewHTTPConnection(context.Background(), "test-server", testServer.URL, customHeaders, nil, "", 0) require.NoError(err) require.NotNil(conn) defer conn.Close() @@ -440,7 +440,7 @@ func TestNewHTTPConnection_EmptyHeaders(t *testing.T) { defer testServer.Close() // Create connection with empty headers - should try SDK transports first - conn, err := NewHTTPConnection(context.Background(), "test-server", testServer.URL, map[string]string{}, nil, "") + conn, err := NewHTTPConnection(context.Background(), "test-server", testServer.URL, map[string]string{}, nil, "", 0) require.NoError(err, "Should succeed with empty headers") require.NotNil(conn) defer conn.Close() @@ -473,7 +473,7 @@ func TestNewHTTPConnection_NilHeaders(t *testing.T) { defer testServer.Close() // Create connection with nil headers (should try SDK transports first) - conn, err := NewHTTPConnection(context.Background(), "test-server", testServer.URL, nil, nil, "") + conn, err := NewHTTPConnection(context.Background(), "test-server", testServer.URL, nil, nil, "", 0) require.NoError(err, "Should succeed with nil headers") require.NotNil(conn) defer conn.Close() @@ -509,7 +509,7 @@ func TestNewHTTPConnection_HTTPClientTimeout(t *testing.T) { // Create connection conn, err := NewHTTPConnection(context.Background(), "test-server", testServer.URL, map[string]string{ "Authorization": "test", - }, nil, "") + }, nil, "", 0) // Should succeed (delay is within timeout) require.NoError(err, "Should succeed within timeout") @@ -530,7 +530,7 @@ func TestNewHTTPConnection_ConnectionRefused(t *testing.T) { // Try to create connection conn, err := NewHTTPConnection(context.Background(), "test-server", unreachableURL, map[string]string{ "Authorization": "test", - }, nil, "") + }, nil, "", 0) // Should fail with connection error require.Error(err, "Should fail with connection refused") @@ -563,7 +563,7 @@ func TestNewHTTPConnection_GettersAfterCreation(t *testing.T) { "X-Custom": "custom-value", } - conn, err := NewHTTPConnection(context.Background(), "test-server", testServer.URL, customHeaders, nil, "") + conn, err := NewHTTPConnection(context.Background(), "test-server", testServer.URL, customHeaders, nil, "", 0) require.NoError(err) require.NotNil(conn) defer conn.Close() diff --git a/internal/mcp/http_error_propagation_test.go b/internal/mcp/http_error_propagation_test.go index eac9df4d..ee05ec21 100644 --- a/internal/mcp/http_error_propagation_test.go +++ b/internal/mcp/http_error_propagation_test.go @@ -119,7 +119,7 @@ func TestHTTPErrorPropagation_Non200Status(t *testing.T) { // Create connection with custom headers to use plain JSON transport conn, err := NewHTTPConnection(context.Background(), "test-server", testServer.URL, map[string]string{ "Authorization": "test-token", - }, nil, "") + }, nil, "", 0) require.NoError(t, err, "Failed to create connection") defer conn.Close() @@ -192,7 +192,7 @@ func TestHTTPErrorPropagation_JSONRPCError(t *testing.T) { conn, err := NewHTTPConnection(context.Background(), "test-server", testServer.URL, map[string]string{ "Authorization": "test-token", - }, nil, "") + }, nil, "", 0) require.NoError(t, err, "Failed to create connection") defer conn.Close() @@ -275,7 +275,7 @@ func TestHTTPErrorPropagation_MixedContent(t *testing.T) { conn, err := NewHTTPConnection(context.Background(), "test-server", testServer.URL, map[string]string{ "Authorization": "test-token", - }, nil, "") + }, nil, "", 0) require.NoError(t, err, "Failed to create connection") defer conn.Close() @@ -344,7 +344,7 @@ func TestHTTPErrorPropagation_PreservesDetails(t *testing.T) { conn, err := NewHTTPConnection(context.Background(), "test-server", testServer.URL, map[string]string{ "Authorization": "test-token", - }, nil, "") + }, nil, "", 0) require.NoError(t, err, "Failed to create connection") defer conn.Close() diff --git a/internal/mcp/http_transport.go b/internal/mcp/http_transport.go index 0f188e92..fa20627a 100644 --- a/internal/mcp/http_transport.go +++ b/internal/mcp/http_transport.go @@ -403,11 +403,12 @@ func trySDKTransport( transportType HTTPTransportType, transportName string, createTransport transportConnector, + keepAlive time.Duration, ) (*Connection, error) { - // Create MCP client with logger and keepalive enabled. - // DefaultHTTPKeepaliveInterval sends periodic pings to prevent session expiry - // on backends (e.g. safeoutputs) that have a 30-minute idle timeout. - client := newMCPClient(logConn, DefaultHTTPKeepaliveInterval) + // Create MCP client with logger and optional keepalive. + // When keepAlive > 0, periodic pings prevent session expiry on backends + // (e.g. safeoutputs) that have an idle timeout. + client := newMCPClient(logConn, keepAlive) // Create transport using the provided connector transport := createTransport(url, httpClient) @@ -422,7 +423,7 @@ func trySDKTransport( return nil, fmt.Errorf("%s transport connect failed: %w", transportName, err) } - conn := newHTTPConnection(ctx, cancel, client, session, url, headers, httpClient, transportType, serverID, DefaultHTTPKeepaliveInterval) + conn := newHTTPConnection(ctx, cancel, client, session, url, headers, httpClient, transportType, serverID, keepAlive) logger.LogInfo("backend", "%s transport connected successfully", transportName) logConn.Printf("Connected with %s transport", transportName) @@ -430,7 +431,7 @@ func trySDKTransport( } // tryStreamableHTTPTransport attempts to connect using the streamable HTTP transport (2025-03-26 spec) -func tryStreamableHTTPTransport(ctx context.Context, cancel context.CancelFunc, serverID, url string, headers map[string]string, httpClient *http.Client) (*Connection, error) { +func tryStreamableHTTPTransport(ctx context.Context, cancel context.CancelFunc, serverID, url string, headers map[string]string, httpClient *http.Client, keepAlive time.Duration) (*Connection, error) { return trySDKTransport( ctx, cancel, serverID, url, headers, httpClient, HTTPTransportStreamable, @@ -442,11 +443,12 @@ func tryStreamableHTTPTransport(ctx context.Context, cancel context.CancelFunc, MaxRetries: 0, // Don't retry on failure - we'll try other transports } }, + keepAlive, ) } // trySSETransport attempts to connect using the SSE transport (2024-11-05 spec) -func trySSETransport(ctx context.Context, cancel context.CancelFunc, serverID, url string, headers map[string]string, httpClient *http.Client) (*Connection, error) { +func trySSETransport(ctx context.Context, cancel context.CancelFunc, serverID, url string, headers map[string]string, httpClient *http.Client, keepAlive time.Duration) (*Connection, error) { return trySDKTransport( ctx, cancel, serverID, url, headers, httpClient, HTTPTransportSSE, @@ -457,6 +459,7 @@ func trySSETransport(ctx context.Context, cancel context.CancelFunc, serverID, u HTTPClient: httpClient, } }, + keepAlive, ) } diff --git a/internal/mcp/http_transport_test.go b/internal/mcp/http_transport_test.go index 4b6897cb..e2f0c592 100644 --- a/internal/mcp/http_transport_test.go +++ b/internal/mcp/http_transport_test.go @@ -686,7 +686,7 @@ func TestSendHTTPRequest_EnsuresToolCallArguments(t *testing.T) { conn, err := NewHTTPConnection(context.Background(), "test-server", testServer.URL, map[string]string{ "Authorization": "test-token", - }, nil, "") + }, nil, "", 0) require.NoError(t, err) require.NotNil(t, conn) defer conn.Close() @@ -738,7 +738,7 @@ func TestSendHTTPRequest_SessionIDFromContext(t *testing.T) { conn, err := NewHTTPConnection(context.Background(), "test-server", testServer.URL, map[string]string{ "Authorization": "test-token", - }, nil, "") + }, nil, "", 0) require.NoError(t, err) require.NotNil(t, conn) defer conn.Close() @@ -843,7 +843,7 @@ func TestSendHTTPRequest_NonToolsCallMethodDoesNotAddArguments(t *testing.T) { conn, err := NewHTTPConnection(context.Background(), "test-server", testServer.URL, map[string]string{ "Authorization": "test-token", - }, nil, "") + }, nil, "", 0) require.NoError(t, err) require.NotNil(t, conn) defer conn.Close() @@ -965,7 +965,7 @@ func TestSendHTTPRequest_ReconnectsOnSessionNotFound(t *testing.T) { conn, err := NewHTTPConnection(context.Background(), "test-server", testServer.URL, map[string]string{ "Authorization": "test-token", - }, nil, "") + }, nil, "", 0) require.NoError(t, err) require.NotNil(t, conn) defer conn.Close() @@ -1026,7 +1026,7 @@ func TestSendHTTPRequest_ReconnectFailure(t *testing.T) { conn, err := NewHTTPConnection(context.Background(), "test-server", testServer.URL, map[string]string{ "Authorization": "test-token", - }, nil, "") + }, nil, "", 0) require.NoError(t, err) require.NotNil(t, conn) defer conn.Close() diff --git a/test/integration/http_error_test.go b/test/integration/http_error_test.go index ea9dd00e..0d89c3ed 100644 --- a/test/integration/http_error_test.go +++ b/test/integration/http_error_test.go @@ -35,7 +35,7 @@ func TestHTTPError_ServerError(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() - conn, err := mcp.NewHTTPConnection(ctx, "test-server", mockServer.URL, nil, nil, "") + conn, err := mcp.NewHTTPConnection(ctx, "test-server", mockServer.URL, nil, nil, "", 0) if err == nil { conn.Close() t.Fatal("Expected connection to fail due to 500 error, but it succeeded") @@ -66,7 +66,7 @@ func TestHTTPError_ClientError(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() - conn, err := mcp.NewHTTPConnection(ctx, "test-server", mockServer.URL, nil, nil, "") + conn, err := mcp.NewHTTPConnection(ctx, "test-server", mockServer.URL, nil, nil, "", 0) if err == nil { conn.Close() t.Fatal("Expected connection to fail due to 401 error, but it succeeded") @@ -108,7 +108,7 @@ func TestHTTPError_ConnectionTimeout(t *testing.T) { defer cancel() startTime := time.Now() - conn, err := mcp.NewHTTPConnection(ctx, "test-server", mockServer.URL, nil, nil, "") + conn, err := mcp.NewHTTPConnection(ctx, "test-server", mockServer.URL, nil, nil, "", 0) elapsed := time.Since(startTime) if err == nil { @@ -143,7 +143,7 @@ func TestHTTPError_ConnectionRefused(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() - conn, err := mcp.NewHTTPConnection(ctx, "test-server", "http://"+addr, nil, nil, "") + conn, err := mcp.NewHTTPConnection(ctx, "test-server", "http://"+addr, nil, nil, "", 0) if err == nil { conn.Close() t.Fatal("Expected connection to fail due to connection refused, but it succeeded") @@ -178,7 +178,7 @@ func TestHTTPError_DroppedConnection(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() - conn, err := mcp.NewHTTPConnection(ctx, "test-server", mockServer.URL, nil, nil, "") + conn, err := mcp.NewHTTPConnection(ctx, "test-server", mockServer.URL, nil, nil, "", 0) if err == nil { conn.Close() t.Fatal("Expected connection to fail due to dropped connection, but it succeeded") @@ -210,7 +210,7 @@ func TestHTTPError_FirewallBlocking(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() - conn, err := mcp.NewHTTPConnection(ctx, "test-server", mockServer.URL, nil, nil, "") + conn, err := mcp.NewHTTPConnection(ctx, "test-server", mockServer.URL, nil, nil, "", 0) if err == nil { conn.Close() t.Fatal("Expected connection to fail due to firewall blocking, but it succeeded") @@ -268,7 +268,7 @@ func TestHTTPError_IntermittentFailure(t *testing.T) { ctx1, cancel1 := context.WithTimeout(context.Background(), 15*time.Second) defer cancel1() - conn1, err1 := mcp.NewHTTPConnection(ctx1, "test-server", mockServer.URL, nil, nil, "") + conn1, err1 := mcp.NewHTTPConnection(ctx1, "test-server", mockServer.URL, nil, nil, "", 0) if err1 == nil { conn1.Close() t.Fatal("Expected first connection to fail, but it succeeded") @@ -279,7 +279,7 @@ func TestHTTPError_IntermittentFailure(t *testing.T) { ctx2, cancel2 := context.WithTimeout(context.Background(), 15*time.Second) defer cancel2() - conn2, err2 := mcp.NewHTTPConnection(ctx2, "test-server", mockServer.URL, nil, nil, "") + conn2, err2 := mcp.NewHTTPConnection(ctx2, "test-server", mockServer.URL, nil, nil, "", 0) if err2 != nil { t.Fatalf("Expected second connection to succeed, but it failed: %v (after %d requests total)", err2, requestCount) } @@ -370,7 +370,7 @@ func TestHTTPError_RequestFailure(t *testing.T) { // Connect successfully ctx := context.Background() - conn, err := mcp.NewHTTPConnection(ctx, "test-server", mockServer.URL, map[string]string{"X-Test": "test"}, nil, "") + conn, err := mcp.NewHTTPConnection(ctx, "test-server", mockServer.URL, map[string]string{"X-Test": "test"}, nil, "", 0) require.NoError(t, err, "Connection failed") defer conn.Close() @@ -410,7 +410,7 @@ func TestHTTPError_MalformedResponse(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() - conn, err := mcp.NewHTTPConnection(ctx, "test-server", mockServer.URL, nil, nil, "") + conn, err := mcp.NewHTTPConnection(ctx, "test-server", mockServer.URL, nil, nil, "", 0) if err == nil { conn.Close() t.Fatal("Expected connection to fail due to malformed response, but it succeeded") @@ -453,7 +453,7 @@ func TestHTTPError_NetworkPartition(t *testing.T) { defer cancel() startTime := time.Now() - conn, err := mcp.NewHTTPConnection(ctx, "test-server", mockServer.URL, nil, nil, "") + conn, err := mcp.NewHTTPConnection(ctx, "test-server", mockServer.URL, nil, nil, "", 0) elapsed := time.Since(startTime) if err == nil { From 1039c17af6937b1d75f68585a83b851716b09f3a Mon Sep 17 00:00:00 2001 From: Landon Cox Date: Thu, 2 Apr 2026 20:14:35 -0700 Subject: [PATCH 5/5] fix: address PR review feedback - Remove session ID from HTTP connection log line to avoid leaking sensitive data (use TruncateSessionID pattern) - Wire keepalive_interval into stdin JSON config path via StdinGatewayConfig.KeepaliveInterval and convertStdinConfig - Add nil guard to GatewayConfig.HTTPKeepaliveInterval() - Close connections during pool eviction instead of just marking state - Make Connection.Close() nil-safe for cancel function - Consolidate duplicate keepalive constants: remove DefaultHTTPKeepaliveInterval from mcp package, use single source of truth in config.DefaultKeepaliveInterval Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- internal/config/config_core.go | 3 +++ internal/config/config_stdin.go | 26 ++++++++++++++------------ internal/launcher/connection_pool.go | 10 ++++++---- internal/mcp/connection.go | 4 +++- internal/mcp/connection_test.go | 23 +++++++++++++---------- internal/mcp/http_transport.go | 7 +------ 6 files changed, 40 insertions(+), 33 deletions(-) diff --git a/internal/config/config_core.go b/internal/config/config_core.go index 7de4c589..519306d7 100644 --- a/internal/config/config_core.go +++ b/internal/config/config_core.go @@ -122,6 +122,9 @@ type GatewayConfig struct { // HTTPKeepaliveInterval returns the keepalive interval as a time.Duration. // A negative KeepaliveInterval disables keepalive (returns 0). func (g *GatewayConfig) HTTPKeepaliveInterval() time.Duration { + if g == nil { + return time.Duration(DefaultKeepaliveInterval) * time.Second + } if g.KeepaliveInterval < 0 { return 0 } diff --git a/internal/config/config_stdin.go b/internal/config/config_stdin.go index c1321ef4..a5865f60 100644 --- a/internal/config/config_stdin.go +++ b/internal/config/config_stdin.go @@ -32,13 +32,14 @@ type StdinConfig struct { // StdinGatewayConfig represents gateway configuration in stdin JSON format. // Uses pointers for optional fields to distinguish between unset and zero values. type StdinGatewayConfig struct { - Port *int `json:"port,omitempty"` - APIKey string `json:"apiKey,omitempty"` - Domain string `json:"domain,omitempty"` - StartupTimeout *int `json:"startupTimeout,omitempty"` - ToolTimeout *int `json:"toolTimeout,omitempty"` - PayloadDir string `json:"payloadDir,omitempty"` - TrustedBots []string `json:"trustedBots,omitempty"` + Port *int `json:"port,omitempty"` + APIKey string `json:"apiKey,omitempty"` + Domain string `json:"domain,omitempty"` + StartupTimeout *int `json:"startupTimeout,omitempty"` + ToolTimeout *int `json:"toolTimeout,omitempty"` + KeepaliveInterval *int `json:"keepaliveInterval,omitempty"` + PayloadDir string `json:"payloadDir,omitempty"` + TrustedBots []string `json:"trustedBots,omitempty"` } // StdinGuardConfig represents a guard configuration in stdin JSON format. @@ -278,11 +279,12 @@ func convertStdinConfig(stdinCfg *StdinConfig) (*Config, error) { // Convert gateway config with defaults if stdinCfg.Gateway != nil { cfg.Gateway = &GatewayConfig{ - Port: intPtrOrDefault(stdinCfg.Gateway.Port, DefaultPort), - APIKey: stdinCfg.Gateway.APIKey, - Domain: stdinCfg.Gateway.Domain, - StartupTimeout: intPtrOrDefault(stdinCfg.Gateway.StartupTimeout, DefaultStartupTimeout), - ToolTimeout: intPtrOrDefault(stdinCfg.Gateway.ToolTimeout, DefaultToolTimeout), + Port: intPtrOrDefault(stdinCfg.Gateway.Port, DefaultPort), + APIKey: stdinCfg.Gateway.APIKey, + Domain: stdinCfg.Gateway.Domain, + StartupTimeout: intPtrOrDefault(stdinCfg.Gateway.StartupTimeout, DefaultStartupTimeout), + ToolTimeout: intPtrOrDefault(stdinCfg.Gateway.ToolTimeout, DefaultToolTimeout), + KeepaliveInterval: intPtrOrDefault(stdinCfg.Gateway.KeepaliveInterval, DefaultKeepaliveInterval), } if stdinCfg.Gateway.PayloadDir != "" { cfg.Gateway.PayloadDir = stdinCfg.Gateway.PayloadDir diff --git a/internal/launcher/connection_pool.go b/internal/launcher/connection_pool.go index 6bcf9a0e..c5943544 100644 --- a/internal/launcher/connection_pool.go +++ b/internal/launcher/connection_pool.go @@ -42,7 +42,7 @@ const ( // DefaultIdleTimeout is the maximum duration a STDIO-backend connection can remain unused // before being removed from the session pool. Set to 6 hours to accommodate long-running // workflow tasks (e.g. ML training, large builds) that may not make MCP calls for extended - // periods. Note: this is distinct from HTTP backend keepalive (DefaultHTTPKeepaliveInterval) + // periods. Note: this is distinct from HTTP backend keepalive (config.DefaultKeepaliveInterval) // which keeps the remote session alive on the HTTP server side; STDIO connections run as local // child processes whose sessions are bounded only by this pool eviction window. DefaultIdleTimeout = 6 * time.Hour @@ -158,10 +158,12 @@ func (p *SessionConnectionPool) cleanupIdleConnections() { logPool.Printf("Cleaning up connection: backend=%s, session=%s, reason=%s, idle=%v, errors=%d", key.BackendID, key.SessionID, reason, now.Sub(metadata.LastUsedAt), metadata.ErrorCount) - // Close the connection if still active + // Close the underlying connection to release resources (cancel context, close SDK session) if metadata.Connection != nil && metadata.State != ConnectionStateClosed { - // Note: mcp.Connection doesn't have a Close method in current implementation - // but we mark it as closed + if err := metadata.Connection.Close(); err != nil { + logPool.Printf("Error closing connection during cleanup: backend=%s, session=%s, err=%v", + key.BackendID, key.SessionID, err) + } metadata.State = ConnectionStateClosed } diff --git a/internal/mcp/connection.go b/internal/mcp/connection.go index 75074188..956cb93c 100644 --- a/internal/mcp/connection.go +++ b/internal/mcp/connection.go @@ -670,7 +670,9 @@ func (c *Connection) getPrompt(params interface{}) (*Response, error) { // Close closes the connection func (c *Connection) Close() error { logConn.Printf("Closing connection: serverID=%s, isHTTP=%v", c.serverID, c.isHTTP) - c.cancel() + if c.cancel != nil { + c.cancel() + } if session := c.getSDKSession(); session != nil { return session.Close() } diff --git a/internal/mcp/connection_test.go b/internal/mcp/connection_test.go index 0d77d203..31b97ba7 100644 --- a/internal/mcp/connection_test.go +++ b/internal/mcp/connection_test.go @@ -521,18 +521,20 @@ func TestNewMCPClientWithLogger(t *testing.T) { // TestNewMCPClientWithKeepalive tests that newMCPClient accepts a keepalive interval func TestNewMCPClientWithKeepalive(t *testing.T) { - client := newMCPClient(nil, DefaultHTTPKeepaliveInterval) + keepAlive := time.Duration(config.DefaultKeepaliveInterval) * time.Second + client := newMCPClient(nil, keepAlive) require.NotNil(t, client, "newMCPClient should return a non-nil client with keepalive") } -// TestDefaultHTTPKeepaliveInterval verifies the keepalive constant is less than a typical +// TestDefaultKeepaliveInterval verifies the config keepalive default is less than a typical // backend session timeout (30 minutes) to prevent session expiry during long agent runs. -func TestDefaultHTTPKeepaliveInterval(t *testing.T) { +func TestDefaultKeepaliveInterval(t *testing.T) { const typicalBackendTimeout = 30 * time.Minute - assert.Less(t, DefaultHTTPKeepaliveInterval, typicalBackendTimeout, - "DefaultHTTPKeepaliveInterval must be less than the typical backend session timeout to prevent expiry") - assert.Greater(t, DefaultHTTPKeepaliveInterval, time.Duration(0), - "DefaultHTTPKeepaliveInterval must be positive") + keepAlive := time.Duration(config.DefaultKeepaliveInterval) * time.Second + assert.Less(t, keepAlive, typicalBackendTimeout, + "DefaultKeepaliveInterval must be less than the typical backend session timeout to prevent expiry") + assert.Greater(t, keepAlive, time.Duration(0), + "DefaultKeepaliveInterval must be positive") } // TestNewHTTPConnectionStoresKeepalive verifies that the keepalive interval is stored on @@ -541,15 +543,16 @@ func TestNewHTTPConnectionStoresKeepalive(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) defer cancel() - client := newMCPClient(nil, DefaultHTTPKeepaliveInterval) + keepAlive := time.Duration(config.DefaultKeepaliveInterval) * time.Second + client := newMCPClient(nil, keepAlive) url := "http://example.com/mcp" headers := map[string]string{} httpClient := &http.Client{} - conn := newHTTPConnection(ctx, cancel, client, nil, url, headers, httpClient, HTTPTransportStreamable, "test-server", DefaultHTTPKeepaliveInterval) + conn := newHTTPConnection(ctx, cancel, client, nil, url, headers, httpClient, HTTPTransportStreamable, "test-server", keepAlive) require.NotNil(t, conn) - assert.Equal(t, DefaultHTTPKeepaliveInterval, conn.keepAliveInterval, + assert.Equal(t, keepAlive, conn.keepAliveInterval, "keepAliveInterval should be stored on the connection for use during reconnection") } diff --git a/internal/mcp/http_transport.go b/internal/mcp/http_transport.go index fa20627a..ab4516fd 100644 --- a/internal/mcp/http_transport.go +++ b/internal/mcp/http_transport.go @@ -36,11 +36,6 @@ const ( // MCPProtocolVersion is the MCP protocol version used in initialization requests. const MCPProtocolVersion = "2025-11-25" -// DefaultHTTPKeepaliveInterval is the default interval for sending keepalive pings to HTTP backends. -// This should be less than the backend's session idle timeout (typically 30 minutes for safeoutputs). -// A value of 25 minutes gives a 5-minute buffer before the session would expire. -const DefaultHTTPKeepaliveInterval = 25 * time.Minute - // requestIDCounter is used to generate unique request IDs for HTTP requests var requestIDCounter uint64 @@ -196,7 +191,7 @@ func newHTTPConnection(ctx context.Context, cancel context.CancelFunc, client *s if session != nil { sessionID = session.ID() } - logHTTP.Printf("Creating HTTP connection: serverID=%s, url=%s, transport=%s, headers=%d, sessionID=%s, keepAlive=%v", serverID, url, transportType, len(headers), sessionID, keepAlive) + logHTTP.Printf("Creating HTTP connection: serverID=%s, url=%s, transport=%s, headers=%d, keepAlive=%v", serverID, url, transportType, len(headers), keepAlive) return &Connection{ client: client, session: session,