diff --git a/CHANGELOG.md b/CHANGELOG.md index cc2fde54..b11bbcf6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- New `--auto-restart` option for the `connect` command: sessions expired by the server are restarted automatically with a fresh MCP session (previous session state is lost), instead of failing until an explicit `restart`. Recovery happens on next use, or in the background whenever sessions are probed (e.g. `mcpc` or `mcpc grep`), same as crashed-session reconnection. + +### Fixed + +- A server-side network failure (e.g. an expired session or unreachable server) no longer makes the CLI needlessly restart a healthy bridge, which could overwrite the session's `expired`/`unauthorized` status with `active` and hide the real state. + ## [0.5.0] - 2026-07-21 ### Changed diff --git a/CLAUDE.md b/CLAUDE.md index e0642e9d..99652162 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -198,7 +198,7 @@ Run `mcpc --help` and `mcpc help ` for the authoritative, always-curren - 🟡 **disconnected** - Bridge process running but server unreachable (lastSeenAt stale >~65s); auto-recovers when server responds - 🟡 **crashed** - Bridge process crashed or killed; auto-reconnects in the background - 🔴 **unauthorized** - Server rejected authentication (401/403) or token refresh failed; requires `login` then `restart` -- 🔴 **expired** - Server rejected session ID (404); requires `restart` +- 🔴 **expired** - Server rejected session ID (404); requires `restart` (sessions created with `connect --auto-restart` are restarted automatically with a fresh session on next use, or in the background whenever sessions are probed, e.g. by `mcpc` or `mcpc grep`) ### Transport Implementation diff --git a/src/cli/commands/connect.ts b/src/cli/commands/connect.ts index 2c84dc3d..3c0c1aa4 100644 --- a/src/cli/commands/connect.ts +++ b/src/cli/commands/connect.ts @@ -134,6 +134,7 @@ type ConnectSessionOptions = { proxyBearerToken?: string; x402?: X402SchemePreference; insecure?: boolean; + autoRestart?: boolean; skipDetails?: boolean; quiet?: boolean; }; @@ -367,6 +368,8 @@ export async function connectSession( ...(proxyConfig && { proxy: proxyConfig }), ...(options.x402 && { x402: options.x402 }), ...(options.insecure && { insecure: true }), + // Persist auto-restart preference (absence leaves an existing session unchanged) + ...(options.autoRestart && { autoRestart: true }), // Clear any previous error status (unauthorized, expired) when reconnecting ...(isReconnect && { status: 'active' }), }; @@ -604,6 +607,7 @@ type BulkConnectOptions = { stdio?: boolean; x402?: X402SchemePreference; insecure?: boolean; + autoRestart?: boolean; }; /** diff --git a/src/cli/commands/sessions.ts b/src/cli/commands/sessions.ts index ec0c04fd..4d7c660a 100644 --- a/src/cli/commands/sessions.ts +++ b/src/cli/commands/sessions.ts @@ -184,7 +184,10 @@ export async function listSessionsAndAuthProfiles(options: { } else if (status === 'crashed') { console.log(chalk.dim(` ↳ run: mcpc ${session.name}`)); } else if (status === 'expired') { - console.log(chalk.dim(` ↳ run: mcpc ${session.name} restart`)); + // No hint for auto-restart sessions — a background restart is already underway + if (!session.autoRestart) { + console.log(chalk.dim(` ↳ run: mcpc ${session.name} restart`)); + } } else if (status === 'disconnected') { // Bridge is alive and auto-recovers when the server responds again; // a restart forces a fresh connection if it stays stuck. diff --git a/src/cli/index.ts b/src/cli/index.ts index cf3324aa..307a5372 100644 --- a/src/cli/index.ts +++ b/src/cli/index.ts @@ -473,6 +473,10 @@ Full docs: ${docsUrl}` .option('--proxy <[host:]port>', 'Start proxy MCP server for session') .option('--proxy-bearer-token ', 'Require authentication for access to proxy server') .option('--stdio', 'Launch all local stdio servers from selected config files') + .option( + '--auto-restart', + 'Restart expired sessions automatically with a fresh session (previous session state is lost)' + ) .option( '--x402 [scheme]', 'Enable x402 auto-payment using the configured wallet; optional scheme: auto (default, prefer upto), upto, or exact.' @@ -500,6 +504,12 @@ ${chalk.bold('Stdio servers (command-based, run locally):')} Config entries spawn the command on connect, even if the handshake later fails — only connect to configs you trust. Bulk connects skip stdio by default; pass --stdio to include them. + +${chalk.bold('Auto-restart (--auto-restart):')} + Expired sessions are restarted automatically with a fresh MCP session + — on next use, or in the background whenever sessions are probed + (e.g. "mcpc", "mcpc grep") — instead of requiring an explicit + "mcpc @session restart". Server-side session state is lost on restart. ${jsonHelp( 'Array of `InitializeResult` objects (one per session), extended with `toolNames` and `_mcpc` metadata', '`[{ protocolVersion?, capabilities?, serverInfo?, instructions?, toolNames?, _mcpc: { sessionName, server?, ... }]`', @@ -530,6 +540,7 @@ ${jsonHelp( ...(opts.proxy && { proxy: opts.proxy as string }), ...(opts.proxyBearerToken && { proxyBearerToken: opts.proxyBearerToken as string }), ...(opts.stdio && { stdio: true }), + ...(opts.autoRestart && { autoRestart: true }), ...(globalOpts.x402 && { x402: globalOpts.x402 }), ...(globalOpts.insecure && { insecure: true }), }); @@ -561,6 +572,7 @@ ${jsonHelp( ...(opts.proxy && { proxy: opts.proxy as string }), ...(opts.proxyBearerToken && { proxyBearerToken: opts.proxyBearerToken as string }), ...(opts.stdio && { stdio: true }), + ...(opts.autoRestart && { autoRestart: true }), ...(globalOpts.x402 && { x402: globalOpts.x402 }), ...(globalOpts.insecure && { insecure: true }), }); @@ -585,6 +597,7 @@ ${jsonHelp( config: parsed.file, proxy: opts.proxy, proxyBearerToken: opts.proxyBearerToken, + ...(opts.autoRestart && { autoRestart: true }), ...(globalOpts.x402 && { x402: globalOpts.x402 }), ...(globalOpts.insecure && { insecure: true }), }); @@ -594,6 +607,7 @@ ${jsonHelp( ...(headers && { headers }), proxy: opts.proxy, proxyBearerToken: opts.proxyBearerToken, + ...(opts.autoRestart && { autoRestart: true }), ...(globalOpts.x402 && { x402: globalOpts.x402 }), ...(globalOpts.insecure && { insecure: true }), }); diff --git a/src/lib/bridge-client.ts b/src/lib/bridge-client.ts index 2041ab22..125f10fe 100644 --- a/src/lib/bridge-client.ts +++ b/src/lib/bridge-client.ts @@ -19,7 +19,14 @@ import { connect, type Socket } from 'net'; import { EventEmitter } from 'events'; import type { IpcMessage, TaskUpdate, X402WalletCredentials } from './types.js'; import { createLogger } from './logger.js'; -import { NetworkError, ClientError, ServerError, AuthError, IpcTimeoutError } from './errors.js'; +import { + NetworkError, + ClientError, + ServerError, + AuthError, + IpcTimeoutError, + markBridgeReported, +} from './errors.js'; import { generateRequestId, sleep } from './utils.js'; const logger = createLogger('bridge-client'); @@ -244,7 +251,9 @@ export class BridgeClient extends EventEmitter { default: error = new Error(message.error.message); } - pending.reject(error); + // Mark as bridge-reported so callers can tell a server-side failure + // (bridge alive) apart from a local socket failure (bridge gone) + pending.reject(markBridgeReported(error)); } else { pending.resolve(message.result); } diff --git a/src/lib/bridge-manager.ts b/src/lib/bridge-manager.ts index aac786e8..ecf2669f 100644 --- a/src/lib/bridge-manager.ts +++ b/src/lib/bridge-manager.ts @@ -30,7 +30,7 @@ import { isSessionExpiredError, enrichErrorMessage, } from './utils.js'; -import { updateSession, getSession } from './sessions.js'; +import { updateSession, getSession, clearSessionMcpSessionId } from './sessions.js'; import { createLogger } from './logger.js'; import { ClientError, @@ -743,13 +743,24 @@ export async function ensureBridgeReady( throw createServerAuthError(target, { sessionName }); } + // Auto-restart recovery: recover an expired session by starting a fresh MCP + // session — drop the rejected session id so the restart below does not attempt + // resumption. There is no bridge worth probing: a session is only marked + // expired right before its bridge shuts down. Previous session state is lost. + let startFresh = false; if (session.status === 'expired') { - throw new ClientError( - `Session ${sessionName} has expired. ` + - `The MCP server indicated the session is no longer valid.\n` + - `To restart the session, run: mcpc ${sessionName} restart\n` + - `To remove the expired session, run: mcpc ${sessionName} close` - ); + if (!session.autoRestart) { + throw new ClientError( + `Session ${sessionName} has expired. ` + + `The MCP server indicated the session is no longer valid.\n` + + `To restart the session, run: mcpc ${sessionName} restart\n` + + `To remove the expired session, run: mcpc ${sessionName} close\n` + + `Tip: sessions created with "mcpc connect --auto-restart" recover from this automatically.` + ); + } + logger.debug(`Session ${sessionName} expired; auto-restart enabled, starting a fresh session`); + await clearSessionMcpSessionId(sessionName); + startFresh = true; } // Socket path is PID-based: each bridge instance gets its own unique path @@ -758,7 +769,7 @@ export async function ensureBridgeReady( // Quick check: is the process alive? const processAlive = session.pid ? isProcessAlive(session.pid) : false; - if (processAlive && socketPath) { + if (!startFresh && processAlive && socketPath) { // Process alive, try getServerDetails (blocks until MCP connected) const result = await checkBridgeHealth(socketPath, timeoutSecs); if (result.healthy) { @@ -777,41 +788,61 @@ export async function ensureBridgeReady( throw new ClientError(enrichErrorMessage(result.error.message, serverUrl)); } } - } else { + } else if (!startFresh) { logger.debug(`Bridge process not alive for ${sessionName}, will try to restart it`); } - // Bridge not healthy - restart it + // Bridge not healthy - restart it (the loop runs at most twice: with + // auto-restart, a restart whose session resumption is rejected by the server + // is retried once with a fresh session). // Use 'connecting' if the session has never successfully connected (no lastSeenAt), // 'reconnecting' if it was previously active. // Set lastConnectionAttemptAt to prevent parallel CLI processes from // also triggering a restart via consolidateSessions/reconnectCrashedSessions. const restartStatus = session.lastSeenAt ? 'reconnecting' : 'connecting'; - await updateSession(sessionName, { - status: restartStatus, - lastConnectionAttemptAt: new Date().toISOString(), - }); - const { pid: newPid } = await restartBridge(sessionName); + for (;;) { + await updateSession(sessionName, { + status: restartStatus, + lastConnectionAttemptAt: new Date().toISOString(), + }); + const { pid: newPid } = await restartBridge(sessionName); - const newSocketPath = getSocketPath(sessionName, newPid); + const newSocketPath = getSocketPath(sessionName, newPid); - // Try getServerDetails on restarted bridge (blocks until MCP connected) - const result = await checkBridgeHealth(newSocketPath, timeoutSecs); - if (result.healthy) { - await updateSession(sessionName, { status: 'active' }); - logger.debug(`Bridge for ${sessionName} passed health check`); - return newSocketPath; - } + // Try getServerDetails on restarted bridge (blocks until MCP connected) + const result = await checkBridgeHealth(newSocketPath, timeoutSecs); + if (result.healthy) { + await updateSession(sessionName, { status: 'active' }); + logger.debug(`Bridge for ${sessionName} passed health check`); + return newSocketPath; + } - // Not healthy after restart - classify the error - const errorMsg = result.error?.message || 'unknown error'; - await classifyAndThrowSessionError(sessionName, session, errorMsg, result.error); + const errorMsg = result.error?.message || 'unknown error'; + + // Auto-restart: if resuming the old MCP session failed because the server + // rejected the session id, retry once with a fresh session (no resumption) + if ( + !startFresh && + session.autoRestart && + isSessionExpiredError(errorMsg, { hadActiveSession: !!session.mcpSessionId }) + ) { + logger.debug( + `Session ${sessionName} expired during restart; auto-restart enabled, retrying with a fresh session` + ); + await clearSessionMcpSessionId(sessionName); + startFresh = true; + continue; + } - // Other errors - provide enriched error with hint to view logs - const serverUrl = session.server.url; - throw new ClientError( - `${enrichErrorMessage(errorMsg, serverUrl)}\n` + `For details, run: mcpc ${sessionName} logs` - ); + // Not healthy after restart - classify the error + await classifyAndThrowSessionError(sessionName, session, errorMsg, result.error); + + // Other errors - provide enriched error with hint to view logs + const serverUrl = session.server.url; + throw new ClientError( + `${enrichErrorMessage(errorMsg, serverUrl)}\n` + `For details, run: mcpc ${sessionName} logs` + ); + } } /** @@ -820,7 +851,9 @@ export async function ensureBridgeReady( * Called after consolidateSessions() identifies crashed sessions eligible for reconnection. * * Unlike explicit "restart" (which creates a fresh MCP session), this preserves - * the existing MCP session ID for resumption when possible. + * the existing MCP session ID for resumption when possible. Expired sessions with + * auto-restart enabled are also included — consolidateSessions() has already + * dropped their rejected session id, so they reconnect with a fresh session. * * @param sessionNames - Names of sessions to reconnect (from consolidateSessions result) */ diff --git a/src/lib/errors.ts b/src/lib/errors.ts index fc5df4a3..4165e490 100644 --- a/src/lib/errors.ts +++ b/src/lib/errors.ts @@ -94,6 +94,38 @@ export class AuthError extends McpError { */ export class IpcTimeoutError extends NetworkError {} +/** + * Marker for errors that the bridge reported over IPC, as opposed to errors + * produced locally in the CLI (e.g. a failed bridge socket). + * + * The distinction matters for NetworkError: a bridge-reported one means the MCP + * *server* was unreachable or rejected the request while the bridge process is + * alive and maintains its own session status (e.g. marking the session expired + * before shutting down). A local one means the bridge itself is gone. Only the + * latter warrants a bridge restart — restarting on the former is pointless and + * races the bridge's own status bookkeeping in sessions.json. + */ +const BRIDGE_REPORTED = Symbol('mcpc.bridgeReported'); + +/** + * Mark an error as reported by the bridge over IPC. Returns the same error. + */ +export function markBridgeReported(error: T): T { + (error as unknown as Record)[BRIDGE_REPORTED] = true; + return error; +} + +/** + * Check whether an error was reported by the bridge over IPC (see markBridgeReported). + */ +export function isBridgeReported(error: unknown): boolean { + return ( + typeof error === 'object' && + error !== null && + (error as Record)[BRIDGE_REPORTED] === true + ); +} + /** * Check if an error message indicates an authentication error from the server. * Uses word boundaries for numeric codes (401, 403) to avoid false positives diff --git a/src/lib/session-client.ts b/src/lib/session-client.ts index 837cb7ad..6e5a36b6 100644 --- a/src/lib/session-client.ts +++ b/src/lib/session-client.ts @@ -33,7 +33,7 @@ import type { ListResourceTemplatesResult } from '@modelcontextprotocol/sdk/type import { BridgeClient } from './bridge-client.js'; import { ensureBridgeReady, restartBridge } from './bridge-manager.js'; import { updateSession } from './sessions.js'; -import { NetworkError, IpcTimeoutError } from './errors.js'; +import { NetworkError, IpcTimeoutError, isBridgeReported } from './errors.js'; import { getSocketPath, generateRequestId } from './utils.js'; import { createLogger } from './logger.js'; @@ -68,9 +68,12 @@ export class SessionClient implements IMcpClient { * 2. Reconnect * 3. Retry the operation once — but only for idempotent operations * - * Two cases are deliberately NOT retried: + * Three cases are deliberately NOT retried: * - IPC timeouts: the bridge is likely healthy and still processing the request; * restarting would kill the in-flight request and retrying could execute it twice. + * - Bridge-reported network errors: the MCP *server* failed, not the bridge — the + * bridge is alive and manages its own session status (e.g. marking the session + * expired), so restarting it would be pointless and would race that bookkeeping. * - Non-idempotent operations (tool calls) after a socket failure: the bridge died * with the request in flight, so the server may already have executed it. We * restart the bridge to recover the session, but surface the uncertainty to the @@ -104,6 +107,15 @@ export class SessionClient implements IMcpClient { throw error; } + // A NetworkError reported by the bridge itself means the MCP *server* was + // unreachable or rejected the request — the bridge process is alive and + // maintains its own session status (e.g. marking the session expired). + // Restarting it here would be pointless and would race that bookkeeping. + if (isBridgeReported(error)) { + error.message = `${error.message}. For details, run: mcpc ${this.sessionName} logs`; + throw error; + } + logger.debug(`Socket error during ${operationName}, will restart bridge...`); // Close the failed client @@ -384,6 +396,13 @@ export class SessionClient implements IMcpClient { throw error; } + // Bridge-reported network error: the server failed, not the bridge — + // don't restart (see withRetry for the full rationale) + if (isBridgeReported(error)) { + error.message = `${error.message}. For details, run: mcpc ${this.sessionName} logs`; + throw error; + } + logger.debug(`Socket error during callToolWithTask, will restart bridge...`); await this.bridgeClient.close(); const { pid: newPid } = await restartBridge(this.sessionName); diff --git a/src/lib/sessions.ts b/src/lib/sessions.ts index 7c1a35e3..f6c32f9a 100644 --- a/src/lib/sessions.ts +++ b/src/lib/sessions.ts @@ -212,6 +212,35 @@ export async function updateSession( ); } +/** + * Remove the stored MCP session id so the next bridge start creates a fresh + * MCP session instead of attempting resumption. Used by auto-restart recovery + * (`connect --auto-restart`) after the server has rejected the old session id. + */ +export async function clearSessionMcpSessionId(sessionName: string): Promise { + const filePath = getSessionsFilePath(); + return withFileLock( + filePath, + async () => { + const storage = await loadSessionsInternal(); + + const session = storage.sessions[sessionName]; + if (!session) { + throw new ClientError(`Session not found: ${sessionName}`); + } + if (session.mcpSessionId === undefined) { + return; + } + + delete session.mcpSessionId; + await saveSessionsInternal(storage); + + logger.debug(`Cleared MCP session id for ${sessionName}`); + }, + SESSIONS_DEFAULT_CONTENT + ); +} + /** * Delete a session * @param sessionName - Name of the session to delete (without @ prefix) @@ -415,9 +444,15 @@ export async function consolidateSessions( // without a profile (e.g. static bearer token via --header) cannot self-heal, so // retrying would just flip the status back to 'connecting' on every `mcpc` call // and hide the real state from the user. + // Expired sessions are included only when created with `connect --auto-restart` — + // their rejected MCP session id is dropped so the restart creates a fresh session. for (const [name, session] of Object.entries(storage.sessions)) { const isRetryableUnauthorized = session?.status === 'unauthorized' && !!session.profileName; - if ((session?.status === 'crashed' || isRetryableUnauthorized) && !session.pid) { + const isAutoRestartExpired = session?.status === 'expired' && !!session.autoRestart; + if ( + (session?.status === 'crashed' || isRetryableUnauthorized || isAutoRestartExpired) && + !session.pid + ) { // Skip if a connection was already attempted within the cooldown window const lastAttempt = session.lastConnectionAttemptAt ? new Date(session.lastConnectionAttemptAt).getTime() @@ -434,6 +469,11 @@ export async function consolidateSessions( continue; } session.lastConnectionAttemptAt = new Date(now).toISOString(); + if (isAutoRestartExpired) { + // The server rejected the old session id — drop it so the restarted + // bridge connects fresh instead of retrying resumption forever. + delete session.mcpSessionId; + } // Use 'connecting' if session has never successfully connected, 'reconnecting' otherwise session.status = session.lastSeenAt ? 'reconnecting' : 'connecting'; hasChanges = true; diff --git a/src/lib/types.ts b/src/lib/types.ts index ae3614e7..aff1e67b 100644 --- a/src/lib/types.ts +++ b/src/lib/types.ts @@ -109,7 +109,8 @@ export interface ProxyConfig { * - connecting: Bridge is starting up for the first time (initial connect in progress) * - reconnecting: Bridge crashed and is being automatically restarted * - unauthorized: Server rejected authentication (401/403) or token refresh failed. Recovery: login then restart. - * - expired: Server indicated session is no longer valid (e.g., 404 response). Recovery: restart. + * - expired: Server indicated session is no longer valid (e.g., 404 response). Recovery: restart + * (automatic for sessions created with `connect --auto-restart`). * - crashed: Bridge process crashed, session might or might not be usable. Bridge will be restarted on next command. */ export type SessionStatus = @@ -157,6 +158,13 @@ export interface SessionData { */ x402?: X402SchemePreference; insecure?: boolean; // Skip TLS certificate verification + /** + * Restart expired sessions automatically (set by `connect --auto-restart`). + * When the server rejects the stored MCP session id, the session is restarted + * with a fresh MCP session instead of failing until an explicit `restart`. + * Previous session state (e.g. added tools, async tasks) is lost on such a restart. + */ + autoRestart?: boolean; pid?: number; // Bridge process PID protocolVersion?: string; // Negotiated MCP version mcpSessionId?: string; // Server-assigned MCP session ID for resumption (stateful Streamable HTTP only) diff --git a/test/e2e/suites/sessions/auto-restart.test.sh b/test/e2e/suites/sessions/auto-restart.test.sh new file mode 100755 index 00000000..d8f610ff --- /dev/null +++ b/test/e2e/suites/sessions/auto-restart.test.sh @@ -0,0 +1,142 @@ +#!/bin/bash +# Test: connect --auto-restart recovers expired sessions automatically + +source "$(dirname "$0")/../../lib/framework.sh" +test_init "sessions/auto-restart" + +# Start test server +start_test_server + +SESSION=$(session_name "auto-restart") + +# Test: connect with --auto-restart stores the flag in sessions.json +test_case "connect --auto-restart stores autoRestart flag" +curl -s -X POST "$TEST_SERVER_URL/control/reset" >/dev/null + +run_mcpc connect "$TEST_SERVER_URL" "$SESSION" --auto-restart +assert_success +_SESSIONS_CREATED+=("$SESSION") + +# Read sessions.json directly: `mcpc --json` consolidates sessions, which would +# kick off background restarts and race the assertions below. +auto_restart=$(jq -r ".sessions[\"$SESSION\"].autoRestart" "$MCPC_HOME_DIR/sessions.json") +if [[ "$auto_restart" != "true" ]]; then + test_fail "expected autoRestart=true in sessions.json, got: $auto_restart" + exit 1 +fi +test_pass + +# Test: capture the MCP session ID before expiry +test_case "session works and MCP session ID is stored" +run_xmcpc "$SESSION" tools-list +assert_success +old_mcp_session_id=$(jq -r ".sessions[\"$SESSION\"].mcpSessionId" "$MCPC_HOME_DIR/sessions.json") +assert_not_empty "$old_mcp_session_id" "mcpSessionId should be stored in sessions.json" +test_pass + +# Test: server-side expiry still fails the in-flight command and marks the session expired +test_case "expired session marks status as expired" +curl -s -X POST "$TEST_SERVER_URL/control/expire-session" >/dev/null + +# The in-flight command fails (server rejects the session); recovery happens on next use +run_mcpc "$SESSION" ping +if [[ "$EXIT_CODE" -eq 0 ]]; then + test_fail "expected command to fail while server rejects the session" + exit 1 +fi + +# The bridge marks the session as expired before shutting down (may take a moment) +for _ in $(seq 1 20); do + session_status=$(jq -r ".sessions[\"$SESSION\"].status" "$MCPC_HOME_DIR/sessions.json") + [[ "$session_status" == "expired" ]] && break + sleep 0.5 +done +if [[ "$session_status" != "expired" ]]; then + test_fail "expected session status to be 'expired' but got '$session_status'" + exit 1 +fi +test_pass + +# Test: next command auto-restarts the expired session with a fresh MCP session +test_case "next command auto-restarts expired session" +# Reset server state so a fresh session can be created +curl -s -X POST "$TEST_SERVER_URL/control/reset" >/dev/null + +run_mcpc "$SESSION" tools-list +assert_success + +session_status=$(jq -r ".sessions[\"$SESSION\"].status" "$MCPC_HOME_DIR/sessions.json") +if [[ "$session_status" != "active" ]]; then + test_fail "expected session status to be 'active' after auto-restart but got '$session_status'" + exit 1 +fi +test_pass + +# Test: the auto-restarted session is a fresh MCP session (old id discarded) +test_case "auto-restart creates a fresh MCP session" +new_mcp_session_id=$(jq -r ".sessions[\"$SESSION\"].mcpSessionId" "$MCPC_HOME_DIR/sessions.json") +assert_not_empty "$new_mcp_session_id" "new mcpSessionId should be stored after auto-restart" +if [[ "$new_mcp_session_id" == "$old_mcp_session_id" ]]; then + test_fail "expected a fresh MCP session id after auto-restart but got the same: $new_mcp_session_id" + exit 1 +fi +echo "MCP session ID changed from $old_mcp_session_id to $new_mcp_session_id" +test_pass + +# Test: probing all sessions (not using the expired one directly) also triggers +# the auto-restart in the background, consistent with crashed-session reconnection. +# This keeps commands like `mcpc grep` working without ever touching the session. +test_case "mcpc grep auto-restarts expired sessions in the background" +curl -s -X POST "$TEST_SERVER_URL/control/expire-session" >/dev/null + +run_mcpc "$SESSION" ping +if [[ "$EXIT_CODE" -eq 0 ]]; then + test_fail "expected ping to fail while server rejects the session" + exit 1 +fi +for _ in $(seq 1 20); do + session_status=$(jq -r ".sessions[\"$SESSION\"].status" "$MCPC_HOME_DIR/sessions.json") + [[ "$session_status" == "expired" ]] && break + sleep 0.5 +done +if [[ "$session_status" != "expired" ]]; then + test_fail "expected session status to be 'expired' but got '$session_status'" + exit 1 +fi + +# Reset server state so a fresh session can be created +curl -s -X POST "$TEST_SERVER_URL/control/reset" >/dev/null + +# Probe sessions repeatedly with grep. The background restart only becomes +# eligible after the auto-restart cooldown (~10s since the bridge was last seen +# alive), so poll until the session recovers. Grep's exit code is irrelevant +# here (1 just means no matches). +recovered="" +for _ in $(seq 1 20); do + run_mcpc grep "echo" + session_status=$(jq -r ".sessions[\"$SESSION\"].status" "$MCPC_HOME_DIR/sessions.json") + if [[ "$session_status" == "active" ]]; then + recovered=1 + break + fi + sleep 2 +done +if [[ -z "$recovered" ]]; then + test_fail "expected mcpc grep to auto-restart the expired session, still '$session_status'" + exit 1 +fi + +# The recovered session must be a fresh MCP session +grep_mcp_session_id=$(jq -r ".sessions[\"$SESSION\"].mcpSessionId" "$MCPC_HOME_DIR/sessions.json") +assert_not_empty "$grep_mcp_session_id" "mcpSessionId should be stored after background auto-restart" +if [[ "$grep_mcp_session_id" == "$new_mcp_session_id" ]]; then + test_fail "expected a fresh MCP session id after background auto-restart but got the same: $grep_mcp_session_id" + exit 1 +fi + +# The session works again without any direct restart +run_xmcpc "$SESSION" tools-list +assert_success +test_pass + +test_done diff --git a/test/unit/lib/session-client.test.ts b/test/unit/lib/session-client.test.ts index 1e1b6cf6..7b462fda 100644 --- a/test/unit/lib/session-client.test.ts +++ b/test/unit/lib/session-client.test.ts @@ -8,10 +8,18 @@ * - A socket failure restarts the bridge, but non-idempotent operations * (tool calls) are NOT re-executed — the server may already have run them. * - Idempotent operations (listTools etc.) are retried once after restart. + * - A NetworkError REPORTED BY the bridge (server unreachable/rejected) never + * restarts the bridge — the bridge is alive and manages its own session + * status (e.g. marking it expired); a restart would race that bookkeeping. */ import { vi } from 'vitest'; -import { NetworkError, IpcTimeoutError, ServerError } from '../../../src/lib/errors.js'; +import { + NetworkError, + IpcTimeoutError, + ServerError, + markBridgeReported, +} from '../../../src/lib/errors.js'; const restartBridge = vi.fn(async () => ({ pid: 4242 })); const updateSession = vi.fn(async () => {}); @@ -106,6 +114,30 @@ describe('SessionClient.withRetry', () => { expect(replacementRequest).toHaveBeenCalledTimes(1); }); + it('does not restart or retry on a bridge-reported network error (idempotent op)', async () => { + // e.g. the server returned 404 for an expired session: the bridge is alive, + // has already marked the session expired, and is shutting down on its own + const bridge = fakeBridgeClient(async () => { + throw markBridgeReported(new NetworkError('Ping failed: 404 Session expired')); + }); + const client = new SessionClient('@test', bridge); + + await expect(client.listTools()).rejects.toThrow(/Session expired/); + expect(restartBridge).not.toHaveBeenCalled(); + expect(replacementRequest).not.toHaveBeenCalled(); + }); + + it('does not restart or retry on a bridge-reported network error (tool call)', async () => { + const bridge = fakeBridgeClient(async () => { + throw markBridgeReported(new NetworkError('fetch failed: server unreachable')); + }); + const client = new SessionClient('@test', bridge); + + await expect(client.callTool('deploy', {})).rejects.toThrow(/server unreachable/); + expect(bridge.request).toHaveBeenCalledTimes(1); + expect(restartBridge).not.toHaveBeenCalled(); + }); + it('does not retry MCP-level errors', async () => { const bridge = fakeBridgeClient(async () => { throw new ServerError('Tool execution failed'); diff --git a/test/unit/lib/sessions.test.ts b/test/unit/lib/sessions.test.ts new file mode 100644 index 00000000..b52e8ed1 --- /dev/null +++ b/test/unit/lib/sessions.test.ts @@ -0,0 +1,156 @@ +/** + * Unit tests for session storage helpers: clearSessionMcpSessionId and the + * auto-restart handling of expired sessions in consolidateSessions. + * + * Drives the real module against a tmp MCPC_HOME_DIR. + */ + +import { mkdtemp, rm } from 'fs/promises'; +import { tmpdir } from 'os'; +import { join } from 'path'; +import { + saveSession, + getSession, + clearSessionMcpSessionId, + consolidateSessions, +} from '../../../src/lib/sessions.js'; +import type { SessionData } from '../../../src/lib/types.js'; + +describe('sessions storage', () => { + let homeDir: string; + let originalHome: string | undefined; + + beforeEach(async () => { + homeDir = await mkdtemp(join(tmpdir(), 'mcpc-sessions-test-')); + originalHome = process.env.MCPC_HOME_DIR; + process.env.MCPC_HOME_DIR = homeDir; + }); + + afterEach(async () => { + if (originalHome === undefined) delete process.env.MCPC_HOME_DIR; + else process.env.MCPC_HOME_DIR = originalHome; + await rm(homeDir, { recursive: true, force: true }); + }); + + function baseSession(overrides: Partial = {}): Omit { + return { + server: { url: 'https://mcp.example.com' }, + createdAt: '2026-01-01T00:00:00Z', + ...overrides, + }; + } + + describe('clearSessionMcpSessionId', () => { + it('removes the stored MCP session id', async () => { + await saveSession('@test', baseSession({ mcpSessionId: 'abc-123' })); + + await clearSessionMcpSessionId('@test'); + + const session = await getSession('@test'); + expect(session).toBeDefined(); + expect(session?.mcpSessionId).toBeUndefined(); + }); + + it('is a no-op when no MCP session id is stored', async () => { + await saveSession('@test', baseSession()); + await expect(clearSessionMcpSessionId('@test')).resolves.toBeUndefined(); + }); + + it('throws for unknown sessions', async () => { + await expect(clearSessionMcpSessionId('@missing')).rejects.toThrow(/Session not found/); + }); + }); + + describe('consolidateSessions auto-restart of expired sessions', () => { + // An old lastConnectionAttemptAt / lastSeenAt, outside the restart cooldown window + const LONG_AGO = '2026-01-01T00:00:00Z'; + + it('marks expired auto-restart sessions for restart and drops the session id', async () => { + await saveSession( + '@expired-auto', + baseSession({ + status: 'expired', + autoRestart: true, + mcpSessionId: 'stale-id', + lastSeenAt: LONG_AGO, + lastConnectionAttemptAt: LONG_AGO, + }) + ); + + const result = await consolidateSessions(false); + + expect(result.sessionsToRestart).toContain('@expired-auto'); + const session = result.sessions['@expired-auto']; + expect(session?.status).toBe('reconnecting'); + // The rejected MCP session id must be dropped so the restarted bridge + // connects fresh instead of retrying resumption forever + expect(session?.mcpSessionId).toBeUndefined(); + + const persisted = await getSession('@expired-auto'); + expect(persisted?.status).toBe('reconnecting'); + expect(persisted?.mcpSessionId).toBeUndefined(); + }); + + it('picks up expired auto-restart sessions whose dead bridge pid is cleared in the same pass', async () => { + // Realistic post-expiry state: the bridge marked the session expired and shut + // down, but its (now dead) pid is still recorded. A single consolidation pass + // must clear the pid AND schedule the restart. + await saveSession( + '@expired-dead-pid', + baseSession({ + status: 'expired', + autoRestart: true, + mcpSessionId: 'stale-id', + pid: 2 ** 30, // beyond Linux pid_max — never a live process + lastSeenAt: LONG_AGO, + lastConnectionAttemptAt: LONG_AGO, + }) + ); + + const result = await consolidateSessions(false); + + expect(result.sessionsToRestart).toContain('@expired-dead-pid'); + const session = result.sessions['@expired-dead-pid']; + expect(session?.pid).toBeUndefined(); + expect(session?.status).toBe('reconnecting'); + expect(session?.mcpSessionId).toBeUndefined(); + }); + + it('leaves expired sessions without auto-restart untouched', async () => { + await saveSession( + '@expired-manual', + baseSession({ + status: 'expired', + mcpSessionId: 'stale-id', + lastSeenAt: LONG_AGO, + lastConnectionAttemptAt: LONG_AGO, + }) + ); + + const result = await consolidateSessions(false); + + expect(result.sessionsToRestart).not.toContain('@expired-manual'); + const session = result.sessions['@expired-manual']; + expect(session?.status).toBe('expired'); + expect(session?.mcpSessionId).toBe('stale-id'); + }); + + it('respects the restart cooldown for expired auto-restart sessions', async () => { + await saveSession( + '@expired-recent', + baseSession({ + status: 'expired', + autoRestart: true, + mcpSessionId: 'stale-id', + lastSeenAt: LONG_AGO, + lastConnectionAttemptAt: new Date().toISOString(), + }) + ); + + const result = await consolidateSessions(false); + + expect(result.sessionsToRestart).not.toContain('@expired-recent'); + expect(result.sessions['@expired-recent']?.status).toBe('expired'); + }); + }); +});