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>

Author: lpcox

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
 	}

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

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
 			}
 

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()
 	}

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

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,