From d97442a273452ba23295117d116af12b9802933c Mon Sep 17 00:00:00 2001 From: JF Date: Mon, 9 Nov 2026 19:59:28 -0500 Subject: [PATCH 1/8] feat(server): debug sessions carry an owner scope; the dispatch gate reads the caller's (#1025) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The owner-key seam of #1025 step 2. Ownership stays server-side ("the handle is the capability"): a session is created in the caller's scope — the authenticated principal when the request carried one, otherwise the shared anonymous scope — a child adopted under it inherits that scope, and list_debug_sessions, resources/list, resources/read, resources/subscribe and the tools/call dispatch answer only within the caller's scope. With no authentication in front of the server every caller is anonymous and sees everything, which is today's single-tenant behaviour; an authenticated http mode later scopes sessions per principal with no tool-schema change. The #939 foreign-session refusal goes with it: there is no "other server" with one core per process, only scopes, and a session outside the caller's scope answers exactly as an unknown id would (no existence leak). The gate still sits in front of every handler, still gates only tools whose schema takes a sessionId, and never refuses create_debug_session, which stamps the scope. Every tool call naming a session also refreshes its tree's activity clock (touchActivity), the input the idle policy of the next commit sweeps on. Co-Authored-By: Claude Fable 5.1 --- src/cli/http-command.ts | 43 ---- src/server.ts | 18 +- src/server/handlers/session-tools.ts | 16 +- src/server/output-resources.ts | 17 +- src/server/owner-key.ts | 39 ++++ src/server/tool-context.ts | 14 +- src/server/tool-dispatch.ts | 44 +++-- src/session/session-manager-core.ts | 58 +++++- src/session/session-store.ts | 11 +- .../core/unit/server/mcp-wire-errors.test.ts | 4 +- tests/core/unit/server/owner-key.test.ts | 35 ++++ tests/core/unit/server/server-test-helpers.ts | 16 ++ .../tool-dispatch-foreign-session.test.ts | 145 -------------- .../server/tool-dispatch-owner-key.test.ts | 186 ++++++++++++++++++ .../unit/session/session-owner-key.test.ts | 80 ++++++++ tests/unit/cli/http-command.test.ts | 65 ------ tests/unit/server-coverage.test.ts | 4 + 17 files changed, 484 insertions(+), 311 deletions(-) create mode 100644 src/server/owner-key.ts create mode 100644 tests/core/unit/server/owner-key.test.ts delete mode 100644 tests/core/unit/server/tool-dispatch-foreign-session.test.ts create mode 100644 tests/core/unit/server/tool-dispatch-owner-key.test.ts create mode 100644 tests/core/unit/session/session-owner-key.test.ts diff --git a/src/cli/http-command.ts b/src/cli/http-command.ts index 194a14c0..8f51b8ee 100644 --- a/src/cli/http-command.ts +++ b/src/cli/http-command.ts @@ -33,39 +33,6 @@ import type { ProcessLike } from '../interfaces/process-interfaces.js'; export type ServerFactoryOptions = DebugMcpServerOptions; -/** How long an idle window is, in the unit a reader thinks in. */ -function describeWindow(ms: number): string { - return ms % 60_000 === 0 ? `${ms / 60_000} min` : `${Math.round(ms / 1000)} s`; -} - -/** - * @internal The answer a tool call gets for a debug session id that another - * HTTP session's server holds (issue #939): whose it is, and how it ends — - * worded from the reaper windows actually in force, so it never promises a - * reaper that is switched off. "About": the sweep runs every - * MCP_HTTP_STALE_SWEEP_INTERVAL_MS, so a close lands up to one interval after - * the window. An SDK client's close() is not a DELETE (only terminateSession() - * sends one), so a plain disconnect leaves the session to the reaper. - */ -export function describeForeignDebugSession( - debugSessionId: string, - windows: { staleMs: number; streamLostMs: number } -): string { - const reaped = [ - windows.streamLostMs > 0 - ? `after about ${describeWindow(windows.streamLostMs)} with that client's SSE stream lost and no reconnect (MCP_HTTP_STREAM_LOST_SESSION_MS=${windows.streamLostMs})` - : undefined, - windows.staleMs > 0 - ? `after about ${describeWindow(windows.staleMs)} idle without a stream (MCP_HTTP_STALE_SESSION_MS=${windows.staleMs})` - : undefined, - ].filter((part): part is string => part !== undefined); - const disconnect = reaped.length > 0 ? 'a plain disconnect leaves it to the idle reaper' : 'a plain disconnect leaves it running'; - const reaper = reaped.length > 0 - ? `the idle reaper closes it ${reaped.join(', or ')}` - : 'no idle reaper is configured (MCP_HTTP_STALE_SESSION_MS and MCP_HTTP_STREAM_LOST_SESSION_MS are 0), so nothing else ends it'; - return `Session ${debugSessionId} belongs to another MCP client session: each Streamable HTTP client sees only the debug sessions it created. Only that client can close it or end its MCP session with an explicit DELETE /mcp (${disconnect}); ${reaper}.`; -} - export interface HttpCommandDependencies { logger: WinstonLoggerType; serverFactory: (options: ServerFactoryOptions) => DebugMcpServer; @@ -249,15 +216,6 @@ export function createHttpApp( // handed this lookup (in handleMcpRequest below). It walks the live map at // call time — nothing to keep in sync, and the asking server, which has // already missed, is harmlessly among the entries. - const describeForeignSession = (debugSessionId: string): string | undefined => { - for (const session of httpSessions.values()) { - if (session.server.sessionManager.getSession(debugSessionId)) { - return describeForeignDebugSession(debugSessionId, { staleMs: staleSessionMs, streamLostMs: streamLostSessionMs }); - } - } - return undefined; - }; - let staleSweepTimer: NodeJS.Timeout | undefined; if (staleSessionMs > 0 || streamLostSessionMs > 0) { const windows = { staleMs: staleSessionMs, streamLostMs: streamLostSessionMs }; @@ -349,7 +307,6 @@ export function createHttpApp( const newDebugServer = serverFactory({ logLevel: options.logLevel, logFile: options.logFile, - describeForeignSession, }); await newDebugServer.start(); // The SDK Server this session speaks through: a legacy-era facade over diff --git a/src/server.ts b/src/server.ts index 1b24fbf5..cfef8d5c 100644 --- a/src/server.ts +++ b/src/server.ts @@ -68,14 +68,6 @@ export type { SetBreakpointRequest }; export interface DebugMcpServerOptions { logLevel?: string; logFile?: string; - /** - * Explains a debug session id this server does not hold when another server - * in the same process does — Streamable HTTP runs one DebugMcpServer per MCP - * client session (issue #939). Answers the sentence the tool call should - * carry, or undefined when the id is simply unknown. Consulted by the - * tools/call dispatch before any handler runs; absent for stdio. - */ - describeForeignSession?: (sessionId: string) => string | undefined; } /** @@ -259,7 +251,7 @@ export class DebugMcpServer implements ToolContext { // Core business logic methods — delegated to by the MCP tool dispatch handler // (CallToolRequestSchema); public so they are also accessible for testing/external use - public async createDebugSession(params: { language: DebugLanguage; name?: string; executablePath?: string; }): Promise { + public async createDebugSession(params: { language: DebugLanguage; name?: string; executablePath?: string; ownerKey?: string; }): Promise { // Validate language support using dynamic discovery const supported = await this.getSupportedLanguagesAsync(); const requested = params.language as unknown as string; @@ -285,7 +277,9 @@ export class DebugMcpServer implements ToolContext { const sessionInfo: DebugSessionInfo = await this.sessionManager.createSession({ language: params.language as DebugLanguage, name: name, - executablePath: params.executablePath // Use executablePath for consistency + executablePath: params.executablePath, // Use executablePath for consistency + // The caller's scope (issue #1025, step 2); absent for the shared anonymous scope. + ...(params.ownerKey !== undefined ? { ownerKey: params.ownerKey } : {}) }); return sessionInfo; } catch (error) { @@ -713,11 +707,7 @@ export class DebugMcpServer implements ToolContext { return this.sessionManager.waitForStop(sessionId, timeoutMs, signal); } - /** @internal ToolContext service; see DebugMcpServerOptions.describeForeignSession. */ - public readonly describeForeignSession?: (sessionId: string) => string | undefined; - constructor(options: DebugMcpServerOptions = {}) { - this.describeForeignSession = options.describeForeignSession; const containerConfig: ContainerConfig = { logLevel: options.logLevel, logFile: options.logFile, diff --git a/src/server/handlers/session-tools.ts b/src/server/handlers/session-tools.ts index eabf25e6..7b331e4c 100644 --- a/src/server/handlers/session-tools.ts +++ b/src/server/handlers/session-tools.ts @@ -13,7 +13,7 @@ import { attachRequestFromArgs, carriesLastStop, emittableStopInfo, hoistStopLan import { namesAttachTarget } from '../../session/attach/attach-addressing.js'; import { failureResult, jsonResult, rethrowAsMcpError, type ToolResult } from '../tool-result.js'; -export const createDebugSessionTool: ToolHandler = async (ctx, args) => { +export const createDebugSessionTool: ToolHandler = async (ctx, args, _toolName, extra) => { // Validate before creating the session so a bad argument does // not leave an orphan session behind (issue #336). assertPlainObjectArg(args.adapterConfig, 'adapterConfig'); @@ -79,7 +79,9 @@ export const createDebugSessionTool: ToolHandler = async (ctx, args) => { const sessionInfo = await ctx.createDebugSession({ language: lang, name: args.name, - executablePath: args.executablePath + executablePath: args.executablePath, + // The caller's scope (issue #1025, step 2); absent for the shared anonymous scope. + ...(extra?.callerKey !== undefined ? { ownerKey: extra.callerKey } : {}) }); // Log session creation @@ -155,9 +157,11 @@ export const createDebugSessionTool: ToolHandler = async (ctx, args) => { } }; -export async function handleListDebugSessions(ctx: ToolContext): Promise { +export async function handleListDebugSessions(ctx: ToolContext, callerKey?: string): Promise { try { - const sessionsInfo: DebugSessionInfo[] = ctx.sessionManager.getAllSessions(); + // The whole process's list, in the caller's scope (issue #1025, step 2): + // with no authentication every caller is anonymous and sees everything. + const sessionsInfo: DebugSessionInfo[] = ctx.sessionManager.getSessionsVisibleTo(callerKey); const sessionData = sessionsInfo.map((session: DebugSessionInfo) => { const mappedSession: Record = { id: session.id, @@ -213,8 +217,8 @@ export async function handleListDebugSessions(ctx: ToolContext): Promise { - return handleListDebugSessions(ctx); +export const listDebugSessionsTool: ToolHandler = async (ctx, _args, _toolName, extra) => { + return handleListDebugSessions(ctx, extra?.callerKey); }; export const closeDebugSessionTool: ToolHandler = async (ctx, args) => { diff --git a/src/server/output-resources.ts b/src/server/output-resources.ts index ef7c84e8..44a78938 100644 --- a/src/server/output-resources.ts +++ b/src/server/output-resources.ts @@ -16,6 +16,7 @@ import type { SessionManager } from '../session/session-manager.js'; import { proxyLogPathFor } from '../proxy/session-log-layout.js'; import { readProxyLogTail } from '../session/launch/proxy-failure-diagnostics.js'; import { outputOnNote } from './handlers/output-tools.js'; +import { ownerKeyOf } from './owner-key.js'; import { outputResourceUri, parseOutputResourceUri, @@ -73,8 +74,9 @@ export function registerResourceHandlers( subscriptions: ResourceSubscriptions, fileSystem: Pick ): void { - server.setRequestHandler('resources/list', async () => { - const sessions = sessionManager.getAllSessions(); + server.setRequestHandler('resources/list', async (_request, callCtx) => { + // The caller's scope (issue #1025, step 2); anonymous sees anonymous. + const sessions = sessionManager.getSessionsVisibleTo(ownerKeyOf(callCtx)); return { resources: sessions.flatMap(session => { const resources = [{ @@ -96,12 +98,15 @@ export function registerResourceHandlers( }; }); - server.setRequestHandler('resources/read', async (request) => { + server.setRequestHandler('resources/read', async (request, callCtx) => { const uri = request.params.uri; const outputSessionId = parseOutputResourceUri(uri); const proxyLogSessionId = parseProxyLogResourceUri(uri); const sessionId = outputSessionId ?? proxyLogSessionId; - const session = sessionId ? sessionManager.getSession(sessionId) : undefined; + // A session outside the caller's scope is an unknown resource (issue #1025, step 2). + const session = sessionId && sessionManager.isSessionVisibleTo(sessionId, ownerKeyOf(callCtx)) + ? sessionManager.getSession(sessionId) + : undefined; if (!session) { throw new WireMcpError(ProtocolErrorCode.InvalidParams, `Unknown resource: ${uri}`); } @@ -133,10 +138,10 @@ export function registerResourceHandlers( }; }); - server.setRequestHandler('resources/subscribe', async (request) => { + server.setRequestHandler('resources/subscribe', async (request, callCtx) => { const uri = request.params.uri; const sessionId = parseOutputResourceUri(uri); - if (!sessionId || !sessionManager.getSession(sessionId)) { + if (!sessionId || !sessionManager.isSessionVisibleTo(sessionId, ownerKeyOf(callCtx))) { throw new WireMcpError(ProtocolErrorCode.InvalidParams, `Unknown resource: ${uri}`); } subscriptions.subscribe(uri); diff --git a/src/server/owner-key.ts b/src/server/owner-key.ts new file mode 100644 index 00000000..500f99e4 --- /dev/null +++ b/src/server/owner-key.ts @@ -0,0 +1,39 @@ +/** + * The owner-key seam (issue #1025, step 2). + * + * Debug-session ownership is server-side and never a tool parameter ("the + * handle is the capability", decided on #1025). Isolation in the sessionless + * 2026-07-28 era is a view keyed by a per-request identity, and the protocol's + * identity is the authenticated principal: this module turns an SDK request + * context into that key. With no authentication in front of the server every + * request is in the shared anonymous scope — one process is single-tenant, + * one process per agent is the isolation — and nothing changes for today's + * users. When an authenticated `http` mode exists, each principal sees only + * its own sessions with no tool-schema change. + * + * The principal claim is `authInfo.clientId`; a deployment that keys on + * another claim changes this one function. + */ + +import type { AuthInfo } from '@modelcontextprotocol/server'; + +/** A debug session's scope: an authenticated principal, or `undefined` for the shared anonymous scope. */ +export type OwnerKey = string | undefined; + +/** The slice of the SDK's request context the seam reads. */ +export interface OwnerKeySource { + http?: { + authInfo?: Partial; + }; +} + +/** The scope a request acts in. An empty principal counts as anonymous. */ +export function ownerKeyOf(callCtx: OwnerKeySource | undefined): OwnerKey { + const clientId = callCtx?.http?.authInfo?.clientId; + return clientId ? clientId : undefined; +} + +/** Whether a session in `ownerKey`'s scope is visible to a caller in `callerKey`'s. */ +export function visibleTo(ownerKey: OwnerKey, callerKey: OwnerKey): boolean { + return ownerKey === callerKey; +} diff --git a/src/server/tool-context.ts b/src/server/tool-context.ts index 9d5e89ed..77855afb 100644 --- a/src/server/tool-context.ts +++ b/src/server/tool-context.ts @@ -72,12 +72,6 @@ export interface ToolContext { getAdapterRegistry(): IAdapterRegistry; getSessionName(sessionId: string): string; validateSession(sessionId: string): void; - /** - * The sentence to answer with when a session id belongs to another server in - * this process (one DebugMcpServer per Streamable HTTP client, issue #939), - * or undefined when the id is simply unknown. Absent for stdio. - */ - describeForeignSession?(sessionId: string): string | undefined; fileNotFoundError(label: string, originalPath: string, fileCheck: FileExistenceResult): ProtocolError; getSupportedLanguagesAsync(): Promise; getLanguageMetadata(): Promise; @@ -87,7 +81,7 @@ export interface ToolContext { validateFunctionBreakpointSupport(sessionId: string): { warning?: string }; // ---- public facade (unchanged signatures) ---- - createDebugSession(params: { language: DebugLanguage; name?: string; executablePath?: string; }): Promise; + createDebugSession(params: { language: DebugLanguage; name?: string; executablePath?: string; ownerKey?: string; }): Promise; startDebugging( sessionId: string, scriptPath: string, @@ -138,6 +132,12 @@ export interface ToolContext { */ export interface ToolCallExtra { signal?: AbortSignal; + /** + * The caller's scope (issue #1025, step 2): the authenticated principal, or + * undefined for the shared anonymous scope. `create_debug_session` stamps it + * on the new session; the dispatch has already refused sessions outside it. + */ + callerKey?: string; } /** diff --git a/src/server/tool-dispatch.ts b/src/server/tool-dispatch.ts index 591207db..cdd7bf27 100644 --- a/src/server/tool-dispatch.ts +++ b/src/server/tool-dispatch.ts @@ -5,7 +5,8 @@ * name produces the same tool:error log line as a failing handler. */ import { Server, ProtocolError, ProtocolErrorCode, type CallToolResult } from '@modelcontextprotocol/server'; -import { toWireError } from '../errors/debug-errors.js'; +import { SessionNotFoundError, toWireError } from '../errors/debug-errors.js'; +import { ownerKeyOf } from './owner-key.js'; import { coerceToolArguments, ToolArguments } from './tool-arguments.js'; import { extractPayloadSuccess, failureResult, sanitizeRequest, type ToolResult } from './tool-result.js'; import { buildToolDefinitions, isToolName, TOOL_ALIASES, type ToolDefinition } from './tool-schemas.js'; @@ -14,31 +15,28 @@ import type { ToolContext } from './tool-context.js'; import { TOOL_HANDLERS } from './handlers/index.js'; /** - * A session id that another server in this process owns answers with that - * fact instead of the tool's own "not found" (Streamable HTTP: one - * DebugMcpServer per MCP client session, issue #939). Decided here, in front - * of every handler, so all tools say it the same way — the per-tool wordings - * (`Session not found`, `Failed to close debug session`, hot_reload' - * protocol error) stay for an id nobody holds. Ownership is terminal, which is - * why it outranks a handler's own argument gates: no argument fix can make the - * call succeed on this server. Only tools whose schema takes a `sessionId` are - * gated — the SDK does not validate arguments against the schema, and a stale - * id an agent carries across MCP sessions must never refuse - * `create_debug_session`. Only the HTTP layer provides the lookup; without it - * nothing changes. + * The owner-key gate (issue #1025, step 2). A `sessionId` argument naming a + * session outside the caller's scope is answered exactly as an unknown id + * would be — the same sentence `SessionNotFoundError` carries — so nothing + * about its existence leaks, and it is decided here, in front of every + * handler, so all tools say it the same way. Only tools whose schema takes a + * `sessionId` are gated: the SDK does not validate arguments against the + * schema, and a stale id an agent carries along must never refuse + * `create_debug_session`. An id nobody holds passes through to the handler's + * own not-found answer. With no authentication configured every caller is in + * the shared anonymous scope and sees everything, as before. */ -function foreignSessionResult(ctx: ToolContext, definition: ToolDefinition, sessionId: unknown): ToolResult | undefined { - if (typeof sessionId !== 'string' || !ctx.describeForeignSession) { +function outOfScopeResult(ctx: ToolContext, definition: ToolDefinition, sessionId: unknown, callerKey: string | undefined): ToolResult | undefined { + if (typeof sessionId !== 'string') { return undefined; } if (!Object.prototype.hasOwnProperty.call(definition.inputSchema.properties ?? {}, 'sessionId')) { return undefined; } - if (ctx.sessionManager.getSession(sessionId)) { + if (!ctx.sessionManager.getSession(sessionId) || ctx.sessionManager.isSessionVisibleTo(sessionId, callerKey)) { return undefined; } - const explanation = ctx.describeForeignSession(sessionId); - return explanation ? failureResult(explanation) : undefined; + return failureResult(new SessionNotFoundError(sessionId).message); } /** @@ -103,9 +101,15 @@ export function registerToolHandlers(server: Server, ctx: ToolContext): void { throw new ProtocolError(ProtocolErrorCode.MethodNotFound, `Unknown tool: ${toolName}`); } assertRequiredToolArguments(definition, args as unknown as Record); + const callerKey = ownerKeyOf(callCtx); + const refused = outOfScopeResult(ctx, definition, args.sessionId, callerKey); + if (!refused && typeof args.sessionId === 'string') { + // A tool call naming a session is activity for the idle policy (issue #1025, step 2). + ctx.sessionManager.touchActivity(args.sessionId); + } const result = - foreignSessionResult(ctx, definition, args.sessionId) ?? - await TOOL_HANDLERS[toolName](ctx, args, requestedName, { signal: callCtx?.mcpReq.signal }); + refused ?? + await TOOL_HANDLERS[toolName](ctx, args, requestedName, { signal: callCtx?.mcpReq.signal, callerKey }); // Log tool response; success mirrors the payload's own success flag (issue #397) ctx.logger.info('tool:response', { diff --git a/src/session/session-manager-core.ts b/src/session/session-manager-core.ts index 8948ab60..168fdebc 100644 --- a/src/session/session-manager-core.ts +++ b/src/session/session-manager-core.ts @@ -3,6 +3,7 @@ * and event handling. */ import { EventEmitter } from 'events'; +import { visibleTo } from '../server/owner-key.js'; import { SessionState, SessionLifecycleState, DebugLanguage, DebugSessionInfo, mapLegacyState, isTerminalSessionState, AdapterPolicy, SessionOutputEntry, redactSecretsInString @@ -399,6 +400,13 @@ export abstract class SessionManagerCore extends EventEmitter { */ protected launchValidationCache = new ValidationResultCache(); + /** + * The idle policy's clocks (issue #1025, step 2): last activity per ROOT + * session id — a tool call naming any session in a tree refreshes its root, + * and only roots expire. Cleared when the root is removed. + */ + private readonly lastActivityAt = new Map(); + /** * Constructor with full dependency injection */ @@ -460,17 +468,58 @@ export abstract class SessionManagerCore extends EventEmitter { return result.redacted ? { text: result.value, redacted: true } : { text, redacted: false }; } - async createSession(params: { language: DebugLanguage; name?: string; executablePath?: string; }): Promise { + async createSession(params: { language: DebugLanguage; name?: string; executablePath?: string; ownerKey?: string; }): Promise { const createParams = { language: params.language, name: params.name, - executablePath: params.executablePath + executablePath: params.executablePath, + // The caller's scope (issue #1025, step 2); absent = the shared anonymous scope. + ...(params.ownerKey !== undefined ? { ownerKey: params.ownerKey } : {}) }; const sessionInfo = this.sessionStore.createSession(createParams); + this.touchActivity(sessionInfo.id); this.logger.info(`[SessionManager] Created new session: ${sessionInfo.name} (ID: ${sessionInfo.id}), state: ${sessionInfo.state}`); return sessionInfo; } + /** + * Whether a caller in `callerKey`'s scope may see and drive this session + * (issue #1025, step 2). An unknown id is visible to nobody. The key lives on + * the managed row only, so the public listing never carries it. + */ + isSessionVisibleTo(sessionId: string, callerKey: string | undefined): boolean { + const session = this.sessionStore.get(sessionId); + return session !== undefined && visibleTo(session.ownerKey, callerKey); + } + + /** The public listing, filtered to the caller's scope (issue #1025, step 2). */ + getSessionsVisibleTo(callerKey: string | undefined): DebugSessionInfo[] { + return this.sessionStore.getAll().filter((info) => visibleTo(this.sessionStore.get(info.id)?.ownerKey, callerKey)); + } + + /** + * Activity for the idle policy (issue #1025, step 2): a tool call naming any + * session in a tree refreshes the tree's root; only roots have a clock. An + * unknown id is a no-op. + */ + touchActivity(sessionId: string): void { + const session = this.sessionStore.get(sessionId); + if (!session) { + return; + } + this.lastActivityAt.set(this.rootOf(session).id, Date.now()); + } + + /** How long a root's tree has been idle, in ms; a child reports its root's. Undefined for an unknown id. */ + idleMsOf(sessionId: string, now = Date.now()): number | undefined { + const session = this.sessionStore.get(sessionId); + if (!session) { + return undefined; + } + const last = this.lastActivityAt.get(this.rootOf(session).id); + return last === undefined ? undefined : Math.max(0, now - last); + } + protected async findFreePort(): Promise { return this.networkManager.findFreePort(); } @@ -804,6 +853,7 @@ export abstract class SessionManagerCore extends EventEmitter { this.logger.info(`Session ${sessionId} marked as STOPPED/TERMINATED.`); this.sessionStore.remove(sessionId); + this.lastActivityAt.delete(sessionId); this.emit('session-removed', sessionId); // Anything still waiting on this session learns it is gone (issue #849), // and a wait on an ancestor's tree sees one descendant fewer (issue #1081). @@ -1104,7 +1154,9 @@ export abstract class SessionManagerCore extends EventEmitter { language: parent.language, name: reported.name, executablePath: parent.executablePath, - parentSessionId: parent.id + parentSessionId: parent.id, + // A child lives in its root's scope (issue #1025, step 2). + ...(parent.ownerKey !== undefined ? { ownerKey: parent.ownerKey } : {}) }); child = this.sessionStore.getOrThrow(info.id); child.logDir = parent.logDir; diff --git a/src/session/session-store.ts b/src/session/session-store.ts index 9fd14d32..edef4834 100644 --- a/src/session/session-store.ts +++ b/src/session/session-store.ts @@ -31,6 +31,12 @@ export interface CreateSessionParams { executablePath?: string; // Language-agnostic executable path /** The session that spawned this one (the session tree, issue #1081); child sessions only. */ parentSessionId?: string; + /** + * The scope the session belongs to (issue #1025, step 2): the authenticated + * principal that created it, or absent for the shared anonymous scope. A + * child inherits its root's. Never projected into DebugSessionInfo. + */ + ownerKey?: string; } import type { DebugProtocol } from '@vscode/debugprotocol'; @@ -103,6 +109,8 @@ export interface SessionExposure { */ export interface ManagedSession extends DebugSessionInfo { executablePath?: string; // Language-agnostic executable path + /** The scope the session belongs to (issue #1025, step 2); see CreateSessionParams.ownerKey. */ + ownerKey?: string; proxyManager?: IProxyManager; // In-flight proxy teardown started by a terminal event handler (which // clears proxyManager before its fire-and-forget stop()); closeSession @@ -260,7 +268,7 @@ export class SessionStore { * Creates a new debug session */ createSession(params: CreateSessionParams): DebugSessionInfo { - const { language, name, executablePath, parentSessionId } = params; + const { language, name, executablePath, parentSessionId, ownerKey } = params; const sessionId = uuidv4(); const sessionName = name || `session-${sessionId.substring(0, 8)}`; @@ -288,6 +296,7 @@ export class SessionStore { sessionLifecycle: SessionLifecycleState.CREATED, executionState: undefined, ...(parentSessionId ? { parentSessionId } : {}), + ...(ownerKey !== undefined ? { ownerKey } : {}), }; this.sessions.set(sessionId, session); diff --git a/tests/core/unit/server/mcp-wire-errors.test.ts b/tests/core/unit/server/mcp-wire-errors.test.ts index ca219dfa..be4b438f 100644 --- a/tests/core/unit/server/mcp-wire-errors.test.ts +++ b/tests/core/unit/server/mcp-wire-errors.test.ts @@ -119,7 +119,9 @@ describe('protocol errors reach SDK clients as the bare reason, with the code be it('an unknown resource (read and subscribe)', async () => { const sessionManager = { getAllSessions: () => [], - getSession: () => undefined + getSessionsVisibleTo: () => [], + getSession: () => undefined, + isSessionVisibleTo: () => false } as unknown as SessionManager; const notifier: ResourceSubscriptions = { subscribe: vi.fn(), unsubscribe: vi.fn() }; registerResourceHandlers(server, sessionManager, notifier, { readTail: vi.fn() }); diff --git a/tests/core/unit/server/owner-key.test.ts b/tests/core/unit/server/owner-key.test.ts new file mode 100644 index 00000000..cbb411e0 --- /dev/null +++ b/tests/core/unit/server/owner-key.test.ts @@ -0,0 +1,35 @@ +/** + * The owner-key seam (issue #1025, step 2): the one place that turns a + * request's identity into the scope a debug session belongs to. With no + * authentication in front of the server every request is in the shared + * anonymous scope, which is today's single-tenant behaviour; an authenticated + * principal scopes its sessions to itself with no tool-schema change. + */ +import { describe, expect, it } from 'vitest'; +import { ownerKeyOf, visibleTo } from '../../../../src/server/owner-key.js'; + +describe('ownerKeyOf', () => { + it('is the authenticated principal when the request carried one', () => { + expect(ownerKeyOf({ http: { authInfo: { clientId: 'agent-a', token: 't', scopes: [] } } })).toBe('agent-a'); + }); + + it('is the shared anonymous scope when nothing authenticated the request', () => { + expect(ownerKeyOf(undefined)).toBeUndefined(); + expect(ownerKeyOf({})).toBeUndefined(); + expect(ownerKeyOf({ http: {} })).toBeUndefined(); + expect(ownerKeyOf({ http: { authInfo: { clientId: '', token: 't', scopes: [] } } })).toBeUndefined(); + }); +}); + +describe('visibleTo', () => { + it('a session is visible to callers in its own scope only', () => { + expect(visibleTo('agent-a', 'agent-a')).toBe(true); + expect(visibleTo('agent-a', 'agent-b')).toBe(false); + expect(visibleTo('agent-a', undefined)).toBe(false); + expect(visibleTo(undefined, 'agent-a')).toBe(false); + }); + + it('the anonymous scope is a scope of its own', () => { + expect(visibleTo(undefined, undefined)).toBe(true); + }); +}); diff --git a/tests/core/unit/server/server-test-helpers.ts b/tests/core/unit/server/server-test-helpers.ts index cd913997..ef656808 100644 --- a/tests/core/unit/server/server-test-helpers.ts +++ b/tests/core/unit/server/server-test-helpers.ts @@ -117,9 +117,25 @@ export function createMockSessionManager(mockAdapterRegistry: IAdapterRegistry) ): ReturnType { return { variables: (await manager.getVariables(...args)) ?? [] }; } + // The owner-key seam's defaults (issue #1025, step 2): the anonymous + // single-tenant view — visibility follows existence, the listing is the + // whole list. Hoisted declarations with explicit return types, like + // delegateToGetVariables, so the self-reference is not a TS7022 circularity. + function sessionVisible(...args: Parameters): ReturnType { + return manager.getSession(args[0]) !== undefined; + } + function visibleSessions(..._args: Parameters): ReturnType { + return manager.getAllSessions() ?? []; + } const manager = { createSession: vi.fn(), getAllSessions: vi.fn(), + // The owner-key seam (issue #1025, step 2): the default is the anonymous + // single-tenant view, every session visible, the listing = getAllSessions. + isSessionVisibleTo: vi.fn(sessionVisible), + getSessionsVisibleTo: vi.fn(visibleSessions), + touchActivity: vi.fn(), + idleMsOf: vi.fn(() => undefined), getSession: vi.fn(), closeSession: vi.fn(), closeSessionReporting: vi.fn(), diff --git a/tests/core/unit/server/tool-dispatch-foreign-session.test.ts b/tests/core/unit/server/tool-dispatch-foreign-session.test.ts deleted file mode 100644 index 153024dd..00000000 --- a/tests/core/unit/server/tool-dispatch-foreign-session.test.ts +++ /dev/null @@ -1,145 +0,0 @@ -/** - * A session id another DebugMcpServer in this process owns answers with that - * fact, whatever tool named it (issue #939). Streamable HTTP runs one server - * per MCP client session; before this gate every tool said a plain - * `Session not found` — or `Failed to close debug session`, or a protocol - * error from hot_reload — and the caller read "gone" while the program - * kept running under a client it could not reach. The gate sits in the - * tools/call dispatch, in front of every handler, consulting the hook the - * HTTP layer injects (`describeForeignSession`); a server without the hook - * (stdio) dispatches exactly as before. - */ -import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; -import { Server, InMemoryTransport } from '@modelcontextprotocol/server'; -import { Client } from '@modelcontextprotocol/client'; -import { registerToolHandlers } from '../../../../src/server/tool-dispatch.js'; -import type { ToolContext } from '../../../../src/server/tool-context.js'; -import { SessionNotFoundError } from '../../../../src/errors/debug-errors.js'; - -const FOREIGN_ID = 'dbg-other'; -const EXPLANATION = `Session ${FOREIGN_ID} belongs to another MCP client session: each Streamable HTTP client sees only the debug sessions it created.`; - -const logger = { info: vi.fn(), error: vi.fn(), warn: vi.fn(), debug: vi.fn() }; -const environment = { - get: (): string | undefined => undefined, - getAll: () => ({}), - getCurrentWorkingDirectory: () => '/' -}; - -function fakeContext(overrides: Partial = {}): ToolContext { - return { - logger, - environment, - getSessionName: () => 'sess', - getSupportedLanguagesAsync: async () => ['mock'], - sessionManager: { getSession: vi.fn(() => undefined) }, - describeForeignSession: vi.fn((sessionId: string) => (sessionId === FOREIGN_ID ? EXPLANATION : undefined)), - validateSession: (sessionId: string) => { - throw new SessionNotFoundError(sessionId); - }, - closeDebugSession: vi.fn(async () => ({ closed: false, closedSessionIds: [] })), - notifications: { forgetUri: vi.fn(), resourcesChanged: vi.fn() }, - ...overrides - } as unknown as ToolContext; -} - -async function connect(server: Server): Promise { - const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); - const client = new Client({ name: 'foreign-session-test', version: '0.0.0' }); - await Promise.all([server.connect(serverTransport), client.connect(clientTransport)]); - return client; -} - -async function answer(client: Client, name: string, args: Record): Promise> { - const raw = await client.callTool({ name, arguments: args }); - const text = (raw.content as Array<{ type: string; text?: string }>)[0]?.text; - return JSON.parse(text!) as Record; -} - -describe('tools/call on a session id another server owns (issue #939)', () => { - let server: Server; - let client: Client; - - beforeEach(() => { - server = new Server({ name: 'foreign-session-server', version: '0.0.0' }, { capabilities: { tools: {} } }); - }); - - afterEach(async () => { - await client?.close(); - await server.close(); - }); - - it('close_debug_session answers the explanation and never reaches the handler', async () => { - const ctx = fakeContext(); - registerToolHandlers(server, ctx); - client = await connect(server); - - expect(await answer(client, 'close_debug_session', { sessionId: FOREIGN_ID })).toEqual({ success: false, error: EXPLANATION }); - expect(ctx.closeDebugSession).not.toHaveBeenCalled(); - }); - - it.each(['step_over', 'get_output', 'wait_for_stop'])('%s answers the explanation instead of its own "not found"', async (tool) => { - registerToolHandlers(server, fakeContext()); - client = await connect(server); - - expect(await answer(client, tool, { sessionId: FOREIGN_ID })).toEqual({ success: false, error: EXPLANATION }); - }); - - it('hot_reload answers the explanation instead of a protocol error', async () => { - registerToolHandlers(server, fakeContext()); - client = await connect(server); - - expect(await answer(client, 'hot_reload', { sessionId: FOREIGN_ID, classesDir: '/classes' })) - .toEqual({ success: false, error: EXPLANATION }); - }); - - it('an id nobody holds keeps the tool\'s own wording', async () => { - const ctx = fakeContext(); - registerToolHandlers(server, ctx); - client = await connect(server); - - expect(await answer(client, 'close_debug_session', { sessionId: 'dbg-nobody' })) - .toEqual({ success: false, message: 'Failed to close debug session: dbg-nobody' }); - expect(ctx.describeForeignSession).toHaveBeenCalledWith('dbg-nobody'); - }); - - it('a session this server holds is dispatched without consulting the other servers', async () => { - const ctx = fakeContext({ - sessionManager: { getSession: vi.fn(() => ({ id: FOREIGN_ID })) } as unknown as ToolContext['sessionManager'], - closeDebugSession: vi.fn(async () => ({ closed: true, closedSessionIds: [FOREIGN_ID] })) - }); - registerToolHandlers(server, ctx); - client = await connect(server); - - expect(await answer(client, 'close_debug_session', { sessionId: FOREIGN_ID })).toMatchObject({ success: true }); - expect(ctx.describeForeignSession).not.toHaveBeenCalled(); - expect(ctx.closeDebugSession).toHaveBeenCalledWith(FOREIGN_ID); - }); - - it('a stray sessionId on a tool whose schema has none (create_debug_session) is never gated', async () => { - // The SDK does not validate arguments against inputSchema, so an agent - // carrying a stale id across MCP sessions can send it anywhere; the one - // tool that must always work must not be refused for it. - const ctx = fakeContext({ - // The launch-toolchain probe fails open when it cannot probe (no registry). - getAdapterRegistry: () => undefined, - createDebugSession: vi.fn(async () => ({ id: 'dbg-new', name: 'fresh', language: 'mock', state: 'created' })) - } as unknown as Partial); - registerToolHandlers(server, ctx); - client = await connect(server); - - expect(await answer(client, 'create_debug_session', { language: 'mock', sessionId: FOREIGN_ID })) - .toMatchObject({ success: true, sessionId: 'dbg-new' }); - expect(ctx.describeForeignSession).not.toHaveBeenCalled(); - }); - - it('a server without the hook (stdio) dispatches as before', async () => { - const ctx = fakeContext({ describeForeignSession: undefined }); - registerToolHandlers(server, ctx); - client = await connect(server); - - expect(await answer(client, 'close_debug_session', { sessionId: FOREIGN_ID })) - .toEqual({ success: false, message: `Failed to close debug session: ${FOREIGN_ID}` }); - expect(ctx.closeDebugSession).toHaveBeenCalledWith(FOREIGN_ID); - }); -}); diff --git a/tests/core/unit/server/tool-dispatch-owner-key.test.ts b/tests/core/unit/server/tool-dispatch-owner-key.test.ts new file mode 100644 index 00000000..44d5c865 --- /dev/null +++ b/tests/core/unit/server/tool-dispatch-owner-key.test.ts @@ -0,0 +1,186 @@ +/** + * The owner-key gate in the tools/call dispatch (issue #1025, step 2). It + * replaces the #939 foreign-session refusal: with one shared core per process + * there is no "other server", only scopes. A session outside the caller's + * scope answers exactly as an unknown id would, so nothing about its existence + * leaks; a session in scope reaches its handler untouched. The caller's scope + * is read off the SDK request context (`ctx.http.authInfo`); with no + * authentication configured every caller is anonymous and sees everything, as + * before. Decided in front of every handler so all tools say it the same way; + * only tools whose schema takes a `sessionId` are gated, and + * `create_debug_session` is never refused — it stamps the caller's scope. + */ +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { registerToolHandlers } from '../../../../src/server/tool-dispatch.js'; +import type { ToolContext } from '../../../../src/server/tool-context.js'; +import { SessionNotFoundError } from '../../../../src/errors/debug-errors.js'; + +// create_debug_session's launch-toolchain gate probes the adapter registry; +// this suite is about the dispatch, so the gate answers "available". +vi.mock('../../../../src/utils/language-availability.js', async (importOriginal) => ({ + ...(await importOriginal()), + checkLaunchToolchain: vi.fn(async () => ({ available: true })) +})); + +const OWN = 'dbg-own'; +const FOREIGN = 'dbg-foreign'; + +const logger = { info: vi.fn(), error: vi.fn(), warn: vi.fn(), debug: vi.fn() }; +const environment = { + get: (): string | undefined => undefined, + getAll: () => ({}), + getCurrentWorkingDirectory: () => '/' +}; + +function fakeContext(overrides: Partial = {}): ToolContext { + return { + logger, + environment, + getSessionName: () => 'sess', + getSupportedLanguagesAsync: async () => ['mock'], + getAdapterRegistry: () => ({}), + validationCache: {}, + sessionManager: { + getSession: vi.fn((id: string) => (id === OWN || id === FOREIGN ? { id } : undefined)), + // OWN belongs to agent-a, FOREIGN to agent-b; the anonymous scope owns nothing here. + isSessionVisibleTo: vi.fn((id: string, key: string | undefined) => + id === OWN ? key === 'agent-a' : id === FOREIGN ? key === 'agent-b' : false), + touchActivity: vi.fn() + }, + validateSession: (sessionId: string) => { + throw new SessionNotFoundError(sessionId); + }, + closeDebugSession: vi.fn(async (id: string) => ({ closed: id === OWN, closedSessionIds: id === OWN ? [OWN] : [] })), + createDebugSession: vi.fn(async () => ({ id: 'dbg-new', name: 'new', language: 'mock', state: 'created', createdAt: new Date() })), + notifications: { forgetUri: vi.fn(), resourcesChanged: vi.fn() }, + ...overrides + } as unknown as ToolContext; +} + +type CallTool = (request: unknown, callCtx?: unknown) => Promise<{ content: Array<{ type: string; text?: string }> }>; + +function dispatcher(ctx: ToolContext): CallTool { + let call: CallTool | undefined; + const recorder = { + setRequestHandler: vi.fn((method: string, handler: unknown) => { + if (method === 'tools/call') { + call = handler as CallTool; + } + }) + }; + registerToolHandlers(recorder as unknown as Parameters[0], ctx); + if (!call) { + throw new Error('tools/call handler not registered'); + } + return call; +} + +/** The SDK's per-request context: an authenticated caller carries authInfo; an anonymous one carries none. */ +const callCtxFor = (clientId?: string) => ({ + mcpReq: { signal: new AbortController().signal }, + ...(clientId !== undefined ? { http: { authInfo: { clientId, token: 't', scopes: [] } } } : {}) +}); + +async function answer(call: CallTool, name: string, args: Record, clientId?: string): Promise> { + const raw = await call({ method: 'tools/call', params: { name, arguments: args } }, callCtxFor(clientId)); + return JSON.parse(raw.content[0].text!) as Record; +} + +describe('tools/call on a session outside the caller scope (issue #1025, step 2)', () => { + afterEach(() => { + vi.clearAllMocks(); + }); + + it('close_debug_session answers exactly as an unknown id and never reaches the handler', async () => { + const ctx = fakeContext(); + const call = dispatcher(ctx); + + const result = await answer(call, 'close_debug_session', { sessionId: FOREIGN }, 'agent-a'); + + expect(result).toEqual({ success: false, error: `Session not found: ${FOREIGN}` }); + expect(ctx.closeDebugSession).not.toHaveBeenCalled(); + }); + + it.each(['step_over', 'get_output', 'wait_for_stop'])('%s answers the same way', async (tool) => { + const call = dispatcher(fakeContext()); + + const result = await answer(call, tool, { sessionId: FOREIGN }, 'agent-a'); + + expect(result).toEqual({ success: false, error: `Session not found: ${FOREIGN}` }); + }); + + it('hot_reload answers the same way instead of its own protocol error', async () => { + const call = dispatcher(fakeContext()); + + const result = await answer(call, 'hot_reload', { sessionId: FOREIGN, classesDir: '/classes' }, 'agent-a'); + + expect(result).toEqual({ success: false, error: `Session not found: ${FOREIGN}` }); + }); + + it('an anonymous caller does not see a keyed session', async () => { + const ctx = fakeContext(); + const call = dispatcher(ctx); + + const result = await answer(call, 'close_debug_session', { sessionId: OWN }); + + expect(result).toEqual({ success: false, error: `Session not found: ${OWN}` }); + expect(ctx.closeDebugSession).not.toHaveBeenCalled(); + }); + + it('a session in the caller scope reaches its handler and refreshes its activity clock', async () => { + const ctx = fakeContext(); + const call = dispatcher(ctx); + + const result = await answer(call, 'close_debug_session', { sessionId: OWN }, 'agent-a'); + + expect(result).toMatchObject({ success: true }); + expect(ctx.closeDebugSession).toHaveBeenCalledWith(OWN); + expect(ctx.sessionManager.touchActivity).toHaveBeenCalledWith(OWN); + }); + + it('an id nobody holds passes through to the handler, which answers its own not-found', async () => { + const ctx = fakeContext(); + const call = dispatcher(ctx); + + const result = await answer(call, 'close_debug_session', { sessionId: 'dbg-nobody' }, 'agent-a'); + + expect(result).toEqual({ success: false, message: 'Failed to close debug session: dbg-nobody' }); + expect(ctx.closeDebugSession).toHaveBeenCalledWith('dbg-nobody'); + }); + + it('create_debug_session is never gated and stamps the caller scope on the new session', async () => { + const ctx = fakeContext(); + const call = dispatcher(ctx); + + const result = await answer(call, 'create_debug_session', { language: 'mock', sessionId: FOREIGN }, 'agent-a'); + + expect(result).toMatchObject({ success: true, sessionId: 'dbg-new' }); + expect(ctx.createDebugSession).toHaveBeenCalledWith(expect.objectContaining({ language: 'mock', ownerKey: 'agent-a' })); + }); + + it('an anonymous create_debug_session carries no key at all', async () => { + const ctx = fakeContext(); + const call = dispatcher(ctx); + + await answer(call, 'create_debug_session', { language: 'mock' }); + + const params = vi.mocked(ctx.createDebugSession).mock.calls[0][0] as Record; + expect(params).not.toHaveProperty('ownerKey'); + }); + + it('with no request context at all (the unit suites), every session is in scope', async () => { + const ctx = fakeContext({ + sessionManager: { + getSession: vi.fn(() => ({ id: OWN })), + isSessionVisibleTo: vi.fn(() => true), + touchActivity: vi.fn() + } as unknown as ToolContext['sessionManager'] + }); + const call = dispatcher(ctx); + + const raw = await call({ method: 'tools/call', params: { name: 'close_debug_session', arguments: { sessionId: OWN } } }); + + expect(JSON.parse(raw.content[0].text!)).toMatchObject({ success: true }); + expect(ctx.sessionManager.isSessionVisibleTo).toHaveBeenCalledWith(OWN, undefined); + }); +}); diff --git a/tests/core/unit/session/session-owner-key.test.ts b/tests/core/unit/session/session-owner-key.test.ts new file mode 100644 index 00000000..182c19cc --- /dev/null +++ b/tests/core/unit/session/session-owner-key.test.ts @@ -0,0 +1,80 @@ +/** + * Debug-session ownership in the core (issue #1025, step 2): a session is + * created in the caller's scope, a child adopted under it inherits that scope, + * and the visibility reads the handlers and resources filter through live + * here. The key itself never reaches the public listing. + */ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { SessionManager, SessionManagerConfig } from '../../../../src/session/session-manager.js'; +import { DebugLanguage } from '@debugmcp/shared'; +import { createMockDependencies } from './session-manager-test-utils.js'; + +const report = (reportId: string, pid = 4242) => ({ + reportId, + targetId: String(pid), + name: `Subprocess ${pid}`, + configuration: { type: 'python', subProcessId: pid } +}); + +describe('SessionManager owner keys (issue #1025, step 2)', () => { + let sessionManager: SessionManager; + let dependencies: ReturnType; + const config: SessionManagerConfig = { + logDirBase: '/tmp/test-sessions', + defaultDapLaunchArgs: { stopOnEntry: false, justMyCode: true } + }; + + beforeEach(() => { + vi.useFakeTimers({ shouldAdvanceTime: true }); + dependencies = createMockDependencies(); + sessionManager = new SessionManager(config, dependencies); + }); + + afterEach(() => { + vi.useRealTimers(); + vi.clearAllMocks(); + dependencies.mockProxyManager.reset(); + }); + + it('a session created in a scope is visible to that scope and to no other', async () => { + const a = await sessionManager.createSession({ language: DebugLanguage.MOCK, ownerKey: 'agent-a' }); + + expect(sessionManager.isSessionVisibleTo(a.id, 'agent-a')).toBe(true); + expect(sessionManager.isSessionVisibleTo(a.id, 'agent-b')).toBe(false); + expect(sessionManager.isSessionVisibleTo(a.id, undefined)).toBe(false); + }); + + it('the anonymous scope sees anonymous sessions only, and an unknown id is visible to nobody', async () => { + const anon = await sessionManager.createSession({ language: DebugLanguage.MOCK }); + const keyed = await sessionManager.createSession({ language: DebugLanguage.MOCK, ownerKey: 'agent-a' }); + + expect(sessionManager.getSessionsVisibleTo(undefined).map((s) => s.id)).toEqual([anon.id]); + expect(sessionManager.getSessionsVisibleTo('agent-a').map((s) => s.id)).toEqual([keyed.id]); + expect(sessionManager.isSessionVisibleTo('nope', undefined)).toBe(false); + expect(sessionManager.isSessionVisibleTo('nope', 'agent-a')).toBe(false); + }); + + it('a child adopted under a keyed root inherits the root scope', async () => { + const root = await sessionManager.createSession({ language: DebugLanguage.MOCK, ownerKey: 'agent-a' }); + await sessionManager.setBreakpoint(root.id, { file: '/w/child.py', line: 7 }); + await sessionManager.startDebugging(root.id, 'test.py'); + await vi.runAllTimersAsync(); + + dependencies.mockProxyManager.simulateEvent('child-reported', report('r1')); + await vi.runAllTimersAsync(); + + const child = sessionManager.getAllSessions().find((s) => s.parentSessionId === root.id); + expect(child).toBeDefined(); + expect(sessionManager.isSessionVisibleTo(child!.id, 'agent-a')).toBe(true); + expect(sessionManager.isSessionVisibleTo(child!.id, undefined)).toBe(false); + expect(sessionManager.getSessionsVisibleTo('agent-a').map((s) => s.id).sort()).toEqual([root.id, child!.id].sort()); + }); + + it('the public listing never carries the key', async () => { + await sessionManager.createSession({ language: DebugLanguage.MOCK, ownerKey: 'agent-a' }); + + for (const row of sessionManager.getAllSessions()) { + expect(row).not.toHaveProperty('ownerKey'); + } + }); +}); diff --git a/tests/unit/cli/http-command.test.ts b/tests/unit/cli/http-command.test.ts index cfe31ecb..8a2d5bc8 100644 --- a/tests/unit/cli/http-command.test.ts +++ b/tests/unit/cli/http-command.test.ts @@ -849,71 +849,6 @@ describe('HTTP Command Handler', () => { }); }); - describe('foreign debug sessions (issue #939)', () => { - // Each HTTP session owns a DebugMcpServer, so a tool call naming a debug - // session another HTTP session's server holds must say so instead of the - // plain "not found" an invalid id gets. The lookup is handed to every new - // server through the factory and walks the live map at call time. - function serverHolding(ids: string[]): DebugMcpServer { - return { - sessionManager: { - getSession: vi.fn((id: string) => (ids.includes(id) ? { id } : undefined)), - getAllSessions: vi.fn(() => ids.map((id) => ({ id }))), - }, - } as unknown as DebugMcpServer; - } - - async function initializedApp(env: Record = {}) { - Object.assign(fakeProc.env, env); - const app = createHttpApp( - { port: '3001' }, - { logger: mockLogger, serverFactory: mockServerFactory, protocolServerFactory: mockProtocolServerFactory, proc: fakeProc } - ); - const handler = mockApp.post.mock.calls.find((c: any) => c[0] === '/mcp')![1]; - await handler( - { - method: 'POST', - headers: {}, - body: { jsonrpc: '2.0', id: 1, method: 'initialize', params: { protocolVersion: '2025-11-25', capabilities: {}, clientInfo: { name: 'c', version: '1' } } }, - }, - { status: vi.fn().mockReturnThis(), json: vi.fn(), end: vi.fn(), headersSent: false } - ); - mockTransport.triggerSessionInit(); - (app as any).httpSessions.set('other-client', { - transport: {}, - server: serverHolding(['dbg-9']), - lastActivity: Date.now(), - openStreams: 1, - hadStream: true, - }); - return mockServerFactory.mock.calls[0][0].describeForeignSession!; - } - - it('hands each new server a lookup that names a debug session another HTTP session holds', async () => { - const describeForeignSession = await initializedApp(); - - expect(typeof describeForeignSession).toBe('function'); - const explanation = describeForeignSession('dbg-9'); - expect(explanation).toMatch(/^Session dbg-9 belongs to another MCP client session: each Streamable HTTP client sees only the debug sessions it created\./); - // Only the owner can end it; an SDK client's close() is not a DELETE, so - // say what a plain disconnect does; the sweep interval makes the window - // approximate. - expect(explanation).toMatch(/Only that client can close it or end its MCP session with an explicit DELETE \/mcp \(a plain disconnect leaves it to the idle reaper\)/); - expect(explanation).toMatch(/after about 2 min with that client's SSE stream lost and no reconnect \(MCP_HTTP_STREAM_LOST_SESSION_MS=120000\)/); - expect(explanation).toMatch(/after about 30 min idle without a stream \(MCP_HTTP_STALE_SESSION_MS=1800000\)/); - expect(describeForeignSession('dbg-nobody')).toBeUndefined(); - }); - - it('does not promise a reaper when both idle windows are off', async () => { - const describeForeignSession = await initializedApp({ MCP_HTTP_STALE_SESSION_MS: '0', MCP_HTTP_STREAM_LOST_SESSION_MS: '0' }); - - const explanation = describeForeignSession('dbg-9')!; - expect(explanation).toMatch(/\(a plain disconnect leaves it running\); no idle reaper is configured \(MCP_HTTP_STALE_SESSION_MS and MCP_HTTP_STREAM_LOST_SESSION_MS are 0\), so nothing else ends it\.$/); - expect(explanation).not.toMatch(/MCP_HTTP_STREAM_LOST_SESSION_MS=/); - expect(explanation).not.toMatch(/leaves it to the idle reaper/); - }); - }); - describe('handleHttpCommand', () => { it('refuses to start on an unusable --allowed-host value, naming it (issue #667)', async () => { await handleHttpCommand( diff --git a/tests/unit/server-coverage.test.ts b/tests/unit/server-coverage.test.ts index 267f11fc..26a96e06 100644 --- a/tests/unit/server-coverage.test.ts +++ b/tests/unit/server-coverage.test.ts @@ -50,6 +50,10 @@ describe('Server Coverage - Error Paths and Edge Cases', () => { closeSessionReporting: vi.fn(), closeAllSessions: vi.fn(), getAllSessions: vi.fn(), + // The owner-key seam (issue #1025, step 2): anonymous view = the whole list. + getSessionsVisibleTo: vi.fn((..._: unknown[]) => mockSessionManager.getAllSessions()), + isSessionVisibleTo: vi.fn((sessionId: string) => mockSessionManager.getSession(sessionId) !== undefined), + touchActivity: vi.fn(), setBreakpoint: vi.fn(), startDebugging: vi.fn(), getVariables: vi.fn(), From b9fcf1979136ace210d25cf1d2c2942da22377fa Mon Sep 17 00:00:00 2001 From: JF Date: Mon, 9 Nov 2026 20:08:42 -0500 Subject: [PATCH 2/8] feat(session): the per-debug-session idle policy (#1025) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The lifetime rule of #1025 step 2, decided on the issue and in the session-tree design: with no protocol-level session to end a debug session, a tree whose root has seen no tool call for the idle window is closed through the normal close cascade (a launch root closes its subtree, an attach root releases its targets). Only roots have a clock; a tool call naming any session in a tree refreshes its root; an in-flight wait_for_stop holds the root; and the sweep is skipped entirely while a client is attending the process (the transport layer supplies that answer; the default says nobody is). - SessionManagerCore: touchActivity, holdIdle, sweepIdle, idleMsOf; clocks and holds are per root and leave with it. - DebugMcpServer: `debugSessionIdle: { defaultMs }` (the transport's default — 30 min for http, 0 for stdio, absent = off), MCP_DEBUG_SESSION_IDLE_MS overriding it (0 disables), MCP_DEBUG_SESSION_IDLE_SWEEP_MS (60 s) for the cadence; invalid values warn and keep the default. start() arms the unref'd sweep, stop() clears it; setAttendedSupplier for the transport layer. - wait_for_stop holds the tree for the duration of the wait and releases in finally, success or failure. The transports adopt it in the next commit. Co-Authored-By: Claude Fable 5.1 --- src/server.ts | 59 ++++++ src/server/handlers/execution-tools.ts | 5 + src/session/idle-policy.ts | 58 ++++++ src/session/session-manager-core.ts | 63 ++++++ .../server/handlers/execution-tools.test.ts | 28 +++ .../core/unit/server/server-lifecycle.test.ts | 115 +++++++++++ tests/core/unit/server/server-test-helpers.ts | 2 + .../unit/session/session-idle-policy.test.ts | 181 ++++++++++++++++++ 8 files changed, 511 insertions(+) create mode 100644 src/session/idle-policy.ts create mode 100644 tests/core/unit/session/session-idle-policy.test.ts diff --git a/src/server.ts b/src/server.ts index cfef8d5c..edcad552 100644 --- a/src/server.ts +++ b/src/server.ts @@ -52,6 +52,7 @@ import { getVariableAccessMode } from './utils/variable-access.js'; import { assertLineContent, resolveStatement, stripTrailingComment } from './utils/breakpoint-resolver.js'; import { outputResourceUri } from './server/output-resources.js'; import { NotificationHub } from './server/notification-hub.js'; +import { describeIdleWindow, resolveIdlePolicy, type IdlePolicy } from './session/idle-policy.js'; import { discoverSupportedLanguages, buildLanguageMetadata, @@ -68,6 +69,13 @@ export type { SetBreakpointRequest }; export interface DebugMcpServerOptions { logLevel?: string; logFile?: string; + /** + * The per-debug-session idle policy's default window (issue #1025, step 2): + * the transport's choice — thirty minutes for `http`, 0 (off) for `stdio`, + * where the pipe is the lifetime. MCP_DEBUG_SESSION_IDLE_MS overrides it. + * Absent = off. + */ + debugSessionIdle?: { defaultMs: number }; } /** @@ -115,6 +123,17 @@ export class DebugMcpServer implements ToolContext { /** @internal ToolContext dependency; read live by the tool handlers. */ public readonly validationCache = new ValidationResultCache(); + /** The idle policy in force (issue #1025, step 2); `idleMs` 0 = off. */ + public readonly idlePolicy: IdlePolicy; + private idleSweepTimer?: NodeJS.Timeout; + private idleSweepInFlight = false; + /** + * Whether a client is attending the process right now (an open legacy SSE + * stream or a modern subscriptions/listen stream): while one is, the sweep + * closes nothing. The transport layer supplies it; the default says nobody is. + */ + private attendedSupplier: () => boolean = () => false; + /** @internal Language discovery is a ToolContext service; see src/server/language-discovery.ts. */ public async getSupportedLanguagesAsync(): Promise { return discoverSupportedLanguages(this.getAdapterRegistry(), this.logger); @@ -742,6 +761,11 @@ export class DebugMcpServer implements ToolContext { this.fileSystem = dependencies.fileSystem; this.version = getVersion(); + this.idlePolicy = resolveIdlePolicy( + this.environment, + { idleMs: options.debugSessionIdle?.defaultMs ?? 0 }, + (message) => this.logger.warn(message) + ); this.instructions = buildServerInstructions(getBpAddressingMode(this.environment), { redactionEnabled: isRedactionEnabled(this.environment), variableAccessMode: getVariableAccessMode(this.environment) @@ -807,9 +831,44 @@ export class DebugMcpServer implements ToolContext { // For MCP servers, start is handled by transport const buildTime = new Date().toISOString(); this.logger.info(`[MCP Server] Started at ${buildTime}, version: ${this.version}`); + // The idle policy's sweep (issue #1025, step 2): armed once per process, + // unref'd so it never holds the event loop open on its own. + if (this.idlePolicy.idleMs > 0 && !this.idleSweepTimer) { + this.idleSweepTimer = setInterval(() => { + void this.runIdleSweep(); + }, this.idlePolicy.sweepMs); + this.idleSweepTimer.unref?.(); + this.logger.info( + `[MCP Server] Debug-session idle policy: a tree with no tool call for ${describeIdleWindow(this.idlePolicy.idleMs)} ` + + `is closed (sweep every ${describeIdleWindow(this.idlePolicy.sweepMs)}; MCP_DEBUG_SESSION_IDLE_MS, MCP_DEBUG_SESSION_IDLE_SWEEP_MS)` + ); + } + } + + /** Who decides whether a client is attending the process; see attendedSupplier. */ + public setAttendedSupplier(supplier: () => boolean): void { + this.attendedSupplier = supplier; + } + + private async runIdleSweep(): Promise { + if (this.idleSweepInFlight) { + return; + } + this.idleSweepInFlight = true; + try { + await this.sessionManager.sweepIdle(this.idlePolicy.idleMs, { attended: this.attendedSupplier() }); + } catch (error) { + this.logger.error('Idle sweep failed', { error }); + } finally { + this.idleSweepInFlight = false; + } } public async stop(): Promise { + if (this.idleSweepTimer) { + clearInterval(this.idleSweepTimer); + this.idleSweepTimer = undefined; + } await this.sessionManager.closeAllSessions(); // Tear down notification bookkeeping (issue #218): pending debounce // timers, the sinks and the SessionManager listeners must not outlive the diff --git a/src/server/handlers/execution-tools.ts b/src/server/handlers/execution-tools.ts index 6e3f9491..0399da7d 100644 --- a/src/server/handlers/execution-tools.ts +++ b/src/server/handlers/execution-tools.ts @@ -129,6 +129,9 @@ export const continueExecutionTool: ToolHandler = async (ctx, args) => { export const waitForStopTool: ToolHandler = async (ctx, args, _toolName, extra) => { requireSessionId(args); + // A wait in flight protects the tree from idle expiry (issue #1025, step 2): + // the agent is attending, even though no tool call lands while it blocks. + const releaseIdleHold = ctx.sessionManager.holdIdle(args.sessionId); try { const waitResult = await ctx.waitForStop(args.sessionId, args.timeout, extra?.signal); if (!waitResult.success) { @@ -187,6 +190,8 @@ export const waitForStopTool: ToolHandler = async (ctx, args, _toolName, extra) return failureResult(error.message); } throw error; + } finally { + releaseIdleHold(); } }; diff --git a/src/session/idle-policy.ts b/src/session/idle-policy.ts new file mode 100644 index 00000000..efe6c4b1 --- /dev/null +++ b/src/session/idle-policy.ts @@ -0,0 +1,58 @@ +/** + * The per-debug-session idle policy's knobs (issue #1025, step 2). + * + * The 2026-07-28 protocol has no session to end a debug session with, so a + * tree whose root has seen no tool call for the window is closed by the core's + * sweep (SessionManagerCore.sweepIdle). The window defaults per transport — + * thirty minutes in `http`, off in `stdio`, where the pipe is the lifetime — + * and MCP_DEBUG_SESSION_IDLE_MS overrides either (`0` disables). The sweep + * cadence is MCP_DEBUG_SESSION_IDLE_SWEEP_MS. Invalid values warn and keep + * the default, the way the HTTP reaper's knobs do. + */ +import type { IEnvironment } from '@debugmcp/shared'; + +export const DEBUG_SESSION_IDLE_ENV = 'MCP_DEBUG_SESSION_IDLE_MS'; +export const DEBUG_SESSION_IDLE_SWEEP_ENV = 'MCP_DEBUG_SESSION_IDLE_SWEEP_MS'; +export const DEFAULT_DEBUG_SESSION_IDLE_SWEEP_MS = 60_000; + +export interface IdlePolicy { + /** The idle window in ms; 0 = the policy is off. */ + readonly idleMs: number; + /** How often the sweep runs, in ms; always > 0. */ + readonly sweepMs: number; +} + +function parseMs( + name: string, + raw: string | undefined, + fallback: number, + warn: (message: string) => void, + minimum: number +): number { + if (raw === undefined || raw.trim() === '') { + return fallback; + } + const parsed = Number(raw); + if (!Number.isFinite(parsed) || parsed < minimum) { + warn(`Ignoring invalid ${name} value "${raw}"; using default ${fallback}ms.`); + return fallback; + } + return parsed; +} + +export function resolveIdlePolicy( + environment: Pick, + defaults: { idleMs: number }, + warn: (message: string) => void +): IdlePolicy { + return { + idleMs: parseMs(DEBUG_SESSION_IDLE_ENV, environment.get(DEBUG_SESSION_IDLE_ENV), defaults.idleMs, warn, 0), + sweepMs: parseMs(DEBUG_SESSION_IDLE_SWEEP_ENV, environment.get(DEBUG_SESSION_IDLE_SWEEP_ENV), DEFAULT_DEBUG_SESSION_IDLE_SWEEP_MS, warn, 1) + }; +} + +/** How long a window is, in the unit a reader thinks in. */ +export function describeIdleWindow(ms: number): string { + if (ms <= 0) return 'off'; + return ms % 60_000 === 0 ? `${ms / 60_000} min` : `${Math.round(ms / 1000)} s`; +} diff --git a/src/session/session-manager-core.ts b/src/session/session-manager-core.ts index 168fdebc..7dafbf62 100644 --- a/src/session/session-manager-core.ts +++ b/src/session/session-manager-core.ts @@ -406,6 +406,8 @@ export abstract class SessionManagerCore extends EventEmitter { * and only roots expire. Cleared when the root is removed. */ private readonly lastActivityAt = new Map(); + /** Roots an in-flight wait_for_stop protects from idle expiry, with the number of holds. */ + private readonly idleHolds = new Map(); /** * Constructor with full dependency injection @@ -510,6 +512,66 @@ export abstract class SessionManagerCore extends EventEmitter { this.lastActivityAt.set(this.rootOf(session).id, Date.now()); } + /** + * Protect a session's tree from idle expiry for the duration of a blocking + * call (wait_for_stop, issue #1025, step 2). Returns the release; releasing + * twice is harmless, and an unknown id yields a no-op pair. + */ + holdIdle(sessionId: string): () => void { + const session = this.sessionStore.get(sessionId); + if (!session) { + return () => undefined; + } + const rootId = this.rootOf(session).id; + this.idleHolds.set(rootId, (this.idleHolds.get(rootId) ?? 0) + 1); + let released = false; + return () => { + if (released) return; + released = true; + const remaining = (this.idleHolds.get(rootId) ?? 1) - 1; + if (remaining > 0) { + this.idleHolds.set(rootId, remaining); + } else { + this.idleHolds.delete(rootId); + } + }; + } + + /** + * The idle policy's sweep (issue #1025, step 2): close every root whose tree + * has seen no tool call for `windowMs` and is not held, through the normal + * close cascade (a launch root closes its subtree, an attach root releases + * its targets). Skipped entirely while a client is attending the process, + * and when the window is 0 (the policy is off). Answers the closed root ids. + */ + async sweepIdle(windowMs: number, options: { now?: number; attended?: boolean } = {}): Promise { + if (windowMs <= 0 || options.attended) { + return []; + } + const now = options.now ?? Date.now(); + const closed: string[] = []; + const roots = this.sessionStore.getAllManaged().filter((session) => !session.parentSessionId); + for (const root of roots) { + const last = this.lastActivityAt.get(root.id); + if (last === undefined) { + // A root with no clock yet (not created through createSession): start it now. + this.lastActivityAt.set(root.id, now); + continue; + } + const idleMs = now - last; + if (idleMs <= windowMs || this.idleHolds.has(root.id)) { + continue; + } + this.logger.info( + `Closing idle debug session ${root.id} (idle ${Math.round(idleMs / 1000)}s): ` + + `no tool call named it or its tree for ${Math.round(windowMs / 1000)}s` + ); + await this.closeSession(root.id); + closed.push(root.id); + } + return closed; + } + /** How long a root's tree has been idle, in ms; a child reports its root's. Undefined for an unknown id. */ idleMsOf(sessionId: string, now = Date.now()): number | undefined { const session = this.sessionStore.get(sessionId); @@ -854,6 +916,7 @@ export abstract class SessionManagerCore extends EventEmitter { this.logger.info(`Session ${sessionId} marked as STOPPED/TERMINATED.`); this.sessionStore.remove(sessionId); this.lastActivityAt.delete(sessionId); + this.idleHolds.delete(sessionId); this.emit('session-removed', sessionId); // Anything still waiting on this session learns it is gone (issue #849), // and a wait on an ancestor's tree sees one descendant fewer (issue #1081). diff --git a/tests/core/unit/server/handlers/execution-tools.test.ts b/tests/core/unit/server/handlers/execution-tools.test.ts index 2a4ac522..64baa540 100644 --- a/tests/core/unit/server/handlers/execution-tools.test.ts +++ b/tests/core/unit/server/handlers/execution-tools.test.ts @@ -84,6 +84,34 @@ describe('execution tool handlers', () => { describe('waitForStopTool (issue #849)', () => { const liveSession = { id: 'test-session', sessionLifecycle: SessionLifecycleState.ACTIVE }; + it('holds the tree against idle expiry for the duration of the wait, then releases it (issue #1025, step 2)', async () => { + ctx.sessionManager.getSession.mockReturnValue(liveSession); + const release = vi.fn(); + ctx.sessionManager.holdIdle.mockReturnValue(release); + let settle: (value: unknown) => void = () => undefined; + ctx.sessionManager.waitForStop.mockReturnValue(new Promise((resolve) => { settle = resolve; })); + + const pending = waitForStopTool(ctx, { sessionId: 'test-session' }, 'wait_for_stop'); + await Promise.resolve(); + expect(ctx.sessionManager.holdIdle).toHaveBeenCalledWith('test-session'); + expect(release).not.toHaveBeenCalled(); + + settle({ success: true, state: 'paused', data: { message: 'Paused', lastStop: { reason: 'pause', threadId: 1, timestamp: 1 } } }); + await pending; + expect(release).toHaveBeenCalledTimes(1); + }); + + it('releases the hold when the wait fails', async () => { + ctx.sessionManager.getSession.mockReturnValue(liveSession); + const release = vi.fn(); + ctx.sessionManager.holdIdle.mockReturnValue(release); + ctx.sessionManager.waitForStop.mockRejectedValue(new Error('proxy gone')); + + await waitForStopTool(ctx, { sessionId: 'test-session' }, 'wait_for_stop').catch(() => undefined); + + expect(release).toHaveBeenCalledTimes(1); + }); + it('hands the timeout and the request signal to the session manager and reports the stop', async () => { ctx.sessionManager.getSession.mockReturnValue(liveSession); const lastStop = { reason: 'breakpoint', threadId: 1, timestamp: 1_700_000_000_000 }; diff --git a/tests/core/unit/server/server-lifecycle.test.ts b/tests/core/unit/server/server-lifecycle.test.ts index ea429f1b..e3714bc4 100644 --- a/tests/core/unit/server/server-lifecycle.test.ts +++ b/tests/core/unit/server/server-lifecycle.test.ts @@ -91,3 +91,118 @@ describe('Server Lifecycle Tests', () => { }); }); + +describe('the debug-session idle policy on the core (issue #1025, step 2)', () => { + let debugServer: DebugMcpServer | undefined; + let mockServer: MockServer; + let mockSessionManager: MockSessionManager; + let mockDependencies: Dependencies; + + beforeEach(() => { + vi.useFakeTimers(); + mockDependencies = createMockDependencies(); + vi.mocked(createProductionDependencies).mockReturnValue(mockDependencies); + mockServer = createMockServer(); + vi.mocked(Server).mockImplementation(function() { return mockServer as any; }); + mockSessionManager = createMockSessionManager(mockDependencies.adapterRegistry); + mockSessionManager.closeAllSessions.mockResolvedValue(undefined); + vi.mocked(SessionManager).mockImplementation(function() { return mockSessionManager as any; }); + debugServer = undefined; + }); + + afterEach(async () => { + await debugServer?.stop(); + vi.useRealTimers(); + vi.unstubAllEnvs(); + vi.clearAllMocks(); + }); + + it('start() arms a sweep at the configured default cadence and stop() clears it', async () => { + debugServer = new DebugMcpServer({ debugSessionIdle: { defaultMs: 30 * 60_000 } }); + expect(debugServer.idlePolicy).toEqual({ idleMs: 1_800_000, sweepMs: 60_000 }); + + await debugServer.start(); + await vi.advanceTimersByTimeAsync(60_000); + expect(mockSessionManager.sweepIdle).toHaveBeenCalledTimes(1); + expect(mockSessionManager.sweepIdle).toHaveBeenCalledWith(1_800_000, { attended: false }); + + await debugServer.stop(); + await vi.advanceTimersByTimeAsync(180_000); + expect(mockSessionManager.sweepIdle).toHaveBeenCalledTimes(1); + }); + + it('a default of 0 (stdio) arms nothing, and so does an absent option', async () => { + debugServer = new DebugMcpServer({ debugSessionIdle: { defaultMs: 0 } }); + await debugServer.start(); + await vi.advanceTimersByTimeAsync(3_600_000); + expect(mockSessionManager.sweepIdle).not.toHaveBeenCalled(); + await debugServer.stop(); + + debugServer = new DebugMcpServer(); + expect(debugServer.idlePolicy.idleMs).toBe(0); + }); + + it('MCP_DEBUG_SESSION_IDLE_MS overrides the default, and 0 disables the policy', async () => { + vi.stubEnv('MCP_DEBUG_SESSION_IDLE_MS', '5000'); + debugServer = new DebugMcpServer({ debugSessionIdle: { defaultMs: 30 * 60_000 } }); + expect(debugServer.idlePolicy.idleMs).toBe(5000); + await debugServer.stop(); + + vi.stubEnv('MCP_DEBUG_SESSION_IDLE_MS', '0'); + debugServer = new DebugMcpServer({ debugSessionIdle: { defaultMs: 30 * 60_000 } }); + expect(debugServer.idlePolicy.idleMs).toBe(0); + await debugServer.start(); + await vi.advanceTimersByTimeAsync(3_600_000); + expect(mockSessionManager.sweepIdle).not.toHaveBeenCalled(); + }); + + it('an invalid window warns and keeps the default', () => { + vi.stubEnv('MCP_DEBUG_SESSION_IDLE_MS', 'soon'); + debugServer = new DebugMcpServer({ debugSessionIdle: { defaultMs: 30 * 60_000 } }); + + expect(debugServer.idlePolicy.idleMs).toBe(1_800_000); + expect(mockDependencies.logger.warn).toHaveBeenCalledWith( + expect.stringContaining('Ignoring invalid MCP_DEBUG_SESSION_IDLE_MS value "soon"') + ); + }); + + it('MCP_DEBUG_SESSION_IDLE_SWEEP_MS sets the cadence; a non-positive value warns and keeps 60 s', async () => { + vi.stubEnv('MCP_DEBUG_SESSION_IDLE_SWEEP_MS', '1000'); + debugServer = new DebugMcpServer({ debugSessionIdle: { defaultMs: 30 * 60_000 } }); + await debugServer.start(); + await vi.advanceTimersByTimeAsync(2_100); + expect(mockSessionManager.sweepIdle).toHaveBeenCalledTimes(2); + await debugServer.stop(); + + vi.stubEnv('MCP_DEBUG_SESSION_IDLE_SWEEP_MS', '0'); + debugServer = new DebugMcpServer({ debugSessionIdle: { defaultMs: 30 * 60_000 } }); + expect(debugServer.idlePolicy.sweepMs).toBe(60_000); + expect(mockDependencies.logger.warn).toHaveBeenCalledWith( + expect.stringContaining('Ignoring invalid MCP_DEBUG_SESSION_IDLE_SWEEP_MS value "0"') + ); + }); + + it('each sweep asks the attended supplier and passes its answer', async () => { + debugServer = new DebugMcpServer({ debugSessionIdle: { defaultMs: 30 * 60_000 } }); + let attended = true; + debugServer.setAttendedSupplier(() => attended); + await debugServer.start(); + + await vi.advanceTimersByTimeAsync(60_000); + expect(mockSessionManager.sweepIdle).toHaveBeenLastCalledWith(1_800_000, { attended: true }); + attended = false; + await vi.advanceTimersByTimeAsync(60_000); + expect(mockSessionManager.sweepIdle).toHaveBeenLastCalledWith(1_800_000, { attended: false }); + }); + + it('a sweep that rejects is logged and the next tick still runs', async () => { + debugServer = new DebugMcpServer({ debugSessionIdle: { defaultMs: 30 * 60_000 } }); + mockSessionManager.sweepIdle.mockRejectedValueOnce(new Error('store exploded')); + await debugServer.start(); + + await vi.advanceTimersByTimeAsync(60_000); + expect(mockDependencies.logger.error).toHaveBeenCalledWith('Idle sweep failed', expect.objectContaining({ error: expect.any(Error) })); + await vi.advanceTimersByTimeAsync(60_000); + expect(mockSessionManager.sweepIdle).toHaveBeenCalledTimes(2); + }); +}); diff --git a/tests/core/unit/server/server-test-helpers.ts b/tests/core/unit/server/server-test-helpers.ts index ef656808..46c0571b 100644 --- a/tests/core/unit/server/server-test-helpers.ts +++ b/tests/core/unit/server/server-test-helpers.ts @@ -136,6 +136,8 @@ export function createMockSessionManager(mockAdapterRegistry: IAdapterRegistry) getSessionsVisibleTo: vi.fn(visibleSessions), touchActivity: vi.fn(), idleMsOf: vi.fn(() => undefined), + holdIdle: vi.fn(() => () => undefined), + sweepIdle: vi.fn(async () => []), getSession: vi.fn(), closeSession: vi.fn(), closeSessionReporting: vi.fn(), diff --git a/tests/core/unit/session/session-idle-policy.test.ts b/tests/core/unit/session/session-idle-policy.test.ts new file mode 100644 index 00000000..72043649 --- /dev/null +++ b/tests/core/unit/session/session-idle-policy.test.ts @@ -0,0 +1,181 @@ +/** + * The per-debug-session idle policy (issue #1025, step 2), decided on the + * issue and in docs/architecture/session-tree.md: with no protocol-level + * session to end a debug session, a tree whose root has seen no tool call for + * the window is closed through the normal close cascade. Only roots have a + * clock (a child goes with its root); a tool call naming any session in the + * tree refreshes the root; an in-flight wait_for_stop holds the root; and the + * sweep is skipped entirely while a client is attending the process. + */ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { SessionManager, SessionManagerConfig } from '../../../../src/session/session-manager.js'; +import { DebugLanguage } from '@debugmcp/shared'; +import { createMockDependencies } from './session-manager-test-utils.js'; + +const report = (reportId: string, pid = 4242) => ({ + reportId, + targetId: String(pid), + name: `Subprocess ${pid}`, + configuration: { type: 'python', subProcessId: pid } +}); + +const WINDOW = 30 * 60 * 1000; + +describe('SessionManager idle policy (issue #1025, step 2)', () => { + let sessionManager: SessionManager; + let dependencies: ReturnType; + const config: SessionManagerConfig = { + logDirBase: '/tmp/test-sessions', + defaultDapLaunchArgs: { stopOnEntry: false, justMyCode: true } + }; + + beforeEach(() => { + vi.useFakeTimers({ shouldAdvanceTime: true }); + vi.setSystemTime(1_000_000); + dependencies = createMockDependencies(); + sessionManager = new SessionManager(config, dependencies); + }); + + afterEach(() => { + vi.useRealTimers(); + vi.clearAllMocks(); + dependencies.mockProxyManager.reset(); + }); + + const now = () => Date.now(); + const ids = () => sessionManager.getAllSessions().map((s) => s.id); + + async function startRoot(): Promise { + const session = await sessionManager.createSession({ language: DebugLanguage.MOCK }); + await sessionManager.setBreakpoint(session.id, { file: '/w/child.py', line: 7 }); + await sessionManager.startDebugging(session.id, 'test.py'); + await vi.runAllTimersAsync(); + return session.id; + } + + async function adoptChild(rootId: string): Promise { + dependencies.mockProxyManager.simulateEvent('child-reported', report('r1')); + await vi.runAllTimersAsync(); + const child = sessionManager.getAllSessions().find((s) => s.parentSessionId === rootId); + expect(child).toBeDefined(); + return child!.id; + } + + it('a root with no activity for the window is closed, and the sweep names it', async () => { + const rootId = await startRoot(); + const closeSpy = vi.spyOn(sessionManager, 'closeSession'); + + const closed = await sessionManager.sweepIdle(WINDOW, { now: now() + WINDOW + 1 }); + + expect(closed).toEqual([rootId]); + expect(closeSpy).toHaveBeenCalledWith(rootId); + expect(ids()).not.toContain(rootId); + expect(dependencies.logger.info).toHaveBeenCalledWith( + expect.stringMatching(new RegExp(`Closing idle debug session ${rootId} \\(idle \\d+s\\)`)) + ); + }); + + it('a root that saw a tool call inside the window is kept', async () => { + const rootId = await startRoot(); + vi.setSystemTime(now() + WINDOW - 1000); + sessionManager.touchActivity(rootId); + + const closed = await sessionManager.sweepIdle(WINDOW, { now: now() + 5000 }); + + expect(closed).toEqual([]); + expect(ids()).toContain(rootId); + }); + + it('a tool call naming a child refreshes the root, so the tree is kept', async () => { + const rootId = await startRoot(); + const childId = await adoptChild(rootId); + vi.setSystemTime(now() + WINDOW - 1000); + sessionManager.touchActivity(childId); + + const closed = await sessionManager.sweepIdle(WINDOW, { now: now() + 5000 }); + + expect(closed).toEqual([]); + expect(ids()).toEqual(expect.arrayContaining([rootId, childId])); + expect(sessionManager.idleMsOf(childId, now() + 5000)).toBe(sessionManager.idleMsOf(rootId, now() + 5000)); + }); + + it('a child under an active root never expires on a clock of its own', async () => { + const rootId = await startRoot(); + const childId = await adoptChild(rootId); + // Nothing ever named the child; the root is active. + vi.setSystemTime(now() + WINDOW * 3); + sessionManager.touchActivity(rootId); + + const closed = await sessionManager.sweepIdle(WINDOW, { now: now() + 1000 }); + + expect(closed).toEqual([]); + expect(ids()).toContain(childId); + }); + + it('the root expiry cascades to the children through the normal close path', async () => { + const rootId = await startRoot(); + const childId = await adoptChild(rootId); + + const closed = await sessionManager.sweepIdle(WINDOW, { now: now() + WINDOW + 1 }); + + expect(closed).toEqual([rootId]); + expect(ids()).not.toContain(rootId); + expect(ids()).not.toContain(childId); + }); + + it('an in-flight wait_for_stop on a child holds the root until it is released', async () => { + const rootId = await startRoot(); + const childId = await adoptChild(rootId); + const release = sessionManager.holdIdle(childId); + + expect(await sessionManager.sweepIdle(WINDOW, { now: now() + WINDOW + 1 })).toEqual([]); + expect(ids()).toContain(rootId); + + release(); + expect(await sessionManager.sweepIdle(WINDOW, { now: now() + WINDOW + 1 })).toEqual([rootId]); + }); + + it('two overlapping holds release independently', async () => { + const rootId = await startRoot(); + const releaseA = sessionManager.holdIdle(rootId); + const releaseB = sessionManager.holdIdle(rootId); + + releaseA(); + releaseA(); // idempotent + expect(await sessionManager.sweepIdle(WINDOW, { now: now() + WINDOW + 1 })).toEqual([]); + + releaseB(); + expect(await sessionManager.sweepIdle(WINDOW, { now: now() + WINDOW + 1 })).toEqual([rootId]); + }); + + it('nothing expires while a client is attending the process; the first unattended sweep closes an idle root', async () => { + const rootId = await startRoot(); + + expect(await sessionManager.sweepIdle(WINDOW, { now: now() + WINDOW * 4, attended: true })).toEqual([]); + expect(ids()).toContain(rootId); + + expect(await sessionManager.sweepIdle(WINDOW, { now: now() + WINDOW * 4, attended: false })).toEqual([rootId]); + }); + + it('a window of zero disables the policy', async () => { + const rootId = await startRoot(); + + expect(await sessionManager.sweepIdle(0, { now: now() + WINDOW * 100 })).toEqual([]); + expect(ids()).toContain(rootId); + }); + + it('holding or touching an unknown session is a no-op', async () => { + expect(() => sessionManager.touchActivity('nope')).not.toThrow(); + const release = sessionManager.holdIdle('nope'); + expect(() => release()).not.toThrow(); + expect(sessionManager.idleMsOf('nope')).toBeUndefined(); + }); + + it('a closed root leaves no clock behind', async () => { + const rootId = await startRoot(); + await sessionManager.closeSession(rootId); + + expect(sessionManager.idleMsOf(rootId)).toBeUndefined(); + expect(await sessionManager.sweepIdle(WINDOW, { now: now() + WINDOW + 1 })).toEqual([]); + }); +}); From df579670aec59665e0e6bc7f1bfb233c1521ba85 Mon Sep 17 00:00:00 2001 From: JF Date: Mon, 9 Nov 2026 20:24:05 -0500 Subject: [PATCH 3/8] feat(http): one debugger core per process; debug sessions outlive their MCP session (#1025) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ownership-and-lifetime change of #1025 step 2 on the http transport. The app builds one DebugMcpServer core after the refusal gates and starts it once; every MCP session is a legacy-era SDK Server facade over it. An MCP session is a connection: its close, reap or DELETE forgets the connection and nothing else — the debug sessions it created belong to the process and answer to the per-debug-session idle policy (30 min default on http, off on stdio). The process is "attended" while any MCP session holds an SSE stream open, and the idle sweep closes nothing then. - /health: `attended`, `listenStreams`, the MCP sessions (`sessions`, `details` without the per-client debug list) and the process's `debugSessions` with their idle time. - Reaper log lines lose the "holding debug session(s)" suffix; the reap closes the transport only. - A transport whose initialize the SDK refused is closed right after the request, so neither it nor its facade is left behind. - Shutdown: every transport, then the one core. - stdio passes the idle default of 0. Tests: the http unit suite's transport mock now completes an initialize inside handleRequest, as the SDK does; http-shared-core.test.ts replaces the #939 foreign-session suite — two clients share one list, either drives the other's session, a session survives its creator's DELETE /mcp — and MEASURES the attendance premise: a v2 SDK client in legacy mode holds its SSE stream for the life of the connection, so /health reports attended with no tool call. The stale-reap e2e now sets the idle knobs and watches the idle policy end the debug session after the MCP-session reap. Docs: README, tool-reference, system-overview, debugging-guide, docker-support, diagnostics, kubernetes (+ sidecar script and smoke workflow), jit-diagnostics, the npm README, SECURITY.md, the session-tree design (root-only clocks, attended suspension), CLAUDE.md; two changelog fragments. Co-Authored-By: Claude Fable 5.1 --- .github/workflows/k8s-smoke.yml | 2 +- CLAUDE.md | 2 +- README.md | 15 +- SECURITY.md | 2 +- changelog.d/1025-idle-policy.added.md | 1 + changelog.d/1025-ownership.changed.md | 1 + docs/architecture/session-tree.md | 49 +++--- docs/architecture/system-overview.md | 2 +- docs/development/debugging-guide.md | 21 ++- docs/diagnostics.md | 4 +- docs/docker-support.md | 28 +-- docs/jit-diagnostics/README.md | 2 +- docs/kubernetes.md | 23 ++- docs/tool-reference.md | 10 +- examples/kubernetes/debug-sidecar.sh | 2 +- packages/mcp-debugger/README.md | 4 +- scripts/k8s-smoke.mjs | 5 +- src/cli/http-command.ts | 156 ++++++++++------- src/cli/stdio-command.ts | 12 +- src/server.ts | 4 +- .../mcp-server-smoke-http-stale-reap.test.ts | 21 ++- .../cli/http-foreign-session.test.ts | 115 ------------- .../cli/http-host-allowlist.test.ts | 12 +- .../integration/cli/http-shared-core.test.ts | 162 ++++++++++++++++++ tests/unit/cli/http-command.test.ts | 158 +++++++++++++---- tests/unit/cli/stdio-command.test.ts | 9 +- 26 files changed, 529 insertions(+), 293 deletions(-) create mode 100644 changelog.d/1025-idle-policy.added.md create mode 100644 changelog.d/1025-ownership.changed.md delete mode 100644 tests/integration/cli/http-foreign-session.test.ts create mode 100644 tests/integration/cli/http-shared-core.test.ts diff --git a/.github/workflows/k8s-smoke.yml b/.github/workflows/k8s-smoke.yml index 4855d7cc..ef553eef 100644 --- a/.github/workflows/k8s-smoke.yml +++ b/.github/workflows/k8s-smoke.yml @@ -145,7 +145,7 @@ jobs: echo "CPP_POD=$POD" >> "$GITHUB_ENV" kubectl debug "$POD" --image="debugmcp/mcp-debugger:${K8S_SMOKE_VERSION}" \ --target=app --profile=general --container=debugger \ - --env=MCP_HTTP_STALE_SESSION_MS=300000 \ + --env=MCP_DEBUG_SESSION_IDLE_MS=300000 --env=MCP_HTTP_STALE_SESSION_MS=300000 \ -- /app/entry.sh http -p 3001 # kubectl debug returns before the image is pulled — wait for the # ephemeral container to actually run. diff --git a/CLAUDE.md b/CLAUDE.md index 478a6fe8..c9f3adda 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -222,7 +222,7 @@ The codebase follows a **layered architecture with dependency injection** and ** - Entry point for MCP protocol communication - `src/server.ts` is the composition root and the shared core (`DebugMcpServer`: dependencies, lifecycle, session validation, breakpoint gating, the public facade methods, the `NotificationHub`); it implements `ToolContext` and passes itself to the handlers. It owns no SDK `Server`: `src/server/protocol-facade.ts` (`createProtocolServer(core, { era, deliver })`) builds one cheap SDK `Server` per connection or request over the core, and `src/server/notification-hub.ts` fans the coalesced `resources/updated` pings and `resources/list_changed` out to every facade's sink (issue #1025) - `src/server/` holds the tool schemas (`tool-schemas.ts`), argument coercion (`tool-arguments.ts`), the tools/call dispatch wrapper (`tool-dispatch.ts` over `handlers/index.ts`), argument validation (`tool-validation.ts`), result envelopes and the session-error catch helpers (`tool-result.ts`), one handler module per tool family (`handlers/*.ts`) over their shared helpers (`handlers/shared.ts`), and the resource (`output-resources.ts`) and prompt (`prompts.ts`) handlers - - Supports STDIO and Streamable HTTP transport modes + - Supports STDIO and Streamable HTTP transport modes. One core per process in both: on `http` every MCP session is a facade over the same debug sessions (the id is the capability; one process per agent is the isolation), a debug session outlives the MCP session that created it, and an abandoned session tree is closed by the per-debug-session idle policy (`MCP_DEBUG_SESSION_IDLE_MS`, 30 min on `http`, off on `stdio`, `0` disables; sweep `MCP_DEBUG_SESSION_IDLE_SWEEP_MS`) unless a client is attending the process (an open SSE stream). The HTTP reapers (`MCP_HTTP_STALE_SESSION_MS`, `MCP_HTTP_STREAM_LOST_SESSION_MS`) reap abandoned MCP sessions only (issue #1025) - Dynamically discovers available language adapters 2. **Adapter System** (NEW) diff --git a/README.md b/README.md index 36c36a40..c9165414 100644 --- a/README.md +++ b/README.md @@ -224,11 +224,16 @@ and point the client at it: } ``` -Each HTTP client gets its own isolated server: debug sessions are never visible to another -client. A client that disconnects without `DELETE /mcp` keeps its debug sessions — and any -paused attach target — alive until the server reaps it: 2 minutes after its SSE stream -dropped (`MCP_HTTP_STREAM_LOST_SESSION_MS`), or 30 minutes idle if it never opened one -(`MCP_HTTP_STALE_SESSION_MS`). `GET /health` lists what each session is holding. +One `http` process is one debugger: every MCP client of it sees the same debug sessions, and a +session is addressable by whoever holds its id (the handle is the capability — run one process +per agent if agents must not see each other's work). A debug session outlives the MCP session +that created it, so a client that reconnects can list and resume or close what it left behind. +What ends an abandoned one is the idle policy: a session tree that no tool call has named for +30 minutes (`MCP_DEBUG_SESSION_IDLE_MS`; `0` disables) is closed, unless a client is attending +the process — while any client holds its SSE stream open, nothing expires. Abandoned MCP +sessions themselves are reaped separately (`MCP_HTTP_STREAM_LOST_SESSION_MS`, +`MCP_HTTP_STALE_SESSION_MS`). `GET /health` lists the MCP sessions, the debug sessions with +their idle time, and whether anyone is attending. The port defaults to 3001, and the server listens on `127.0.0.1` only. `--bind 0.0.0.0` (or `MCP_HTTP_BIND=0.0.0.0`) listens on every interface — pair it with `--allowed-host` for the names other diff --git a/SECURITY.md b/SECURITY.md index 952e5a2b..4ee4d571 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -68,7 +68,7 @@ If you want to constrain what the MCP client can reach, constrain the mcp-debugg - **Containers.** Run the published `debugmcp/mcp-debugger` image with the workspace mounted at `/workspace`. The container boundary is the filesystem boundary. - **OS permissions.** Run mcp-debugger as a user with read access only to the source you want it to debug, and no access to secrets, SSH keys, credentials, etc. The kernel enforces the limits. -- **Network exposure.** If running in http mode, treat the port as a remote shell endpoint — anyone who can reach it inherits the mcp-debugger process's privileges. Bind to loopback or put it behind authenticated network access. +- **Network exposure.** If running in http mode, treat the port as a remote shell endpoint — anyone who can reach it inherits the mcp-debugger process's privileges. Bind to loopback or put it behind authenticated network access. One http process is single-tenant: every client of it sees and can drive every debug session (a session id is the capability); run one process per agent where that matters. With authentication in front of `/mcp`, debug sessions are scoped to the authenticated principal that created them. - **Remote debuggees.** When using `attach_to_process` to debug a process on another host, the MCP client inherits that debuggee's privileges too. Apply the same containment reasoning there. - **MCP client trust.** Do not expose mcp-debugger to an MCP client that you trust less than the OS user mcp-debugger runs under. - **DAP mirror endpoints.** `expose_session` opens a per-session DAP endpoint bound to `127.0.0.1` and gated by a random per-expose token. The mirror rejects execution control and breakpoint changes, but it forwards `evaluate` — and DAP evaluate can execute arbitrary code in the debuggee — so **treat the mirror token as a debuggee-execution capability, not a view-only credential**. Any local process that obtains the token gets that capability; the token is returned only in the `expose_session` result and is redacted from logs. "Read-only" means control is rejected, not that the debuggee is immutable. diff --git a/changelog.d/1025-idle-policy.added.md b/changelog.d/1025-idle-policy.added.md new file mode 100644 index 00000000..94430aea --- /dev/null +++ b/changelog.d/1025-idle-policy.added.md @@ -0,0 +1 @@ +**A per-debug-session idle policy replaces the MCP-session reaper as what ends an abandoned debug session.** On `http`, a debug-session tree that no tool call has named for `MCP_DEBUG_SESSION_IDLE_MS` (default 30 minutes; `0` disables) is closed through the normal close cascade — a launch root closes its subtree, an attach root releases its targets — unless a client is attending the process (an open SSE stream), in which case nothing expires; a tool call naming any session in a tree refreshes the whole tree, an in-flight `wait_for_stop` holds it, and children never expire on a clock of their own. The sweep runs every `MCP_DEBUG_SESSION_IDLE_SWEEP_MS` (default 60 s). On `stdio` the pipe is the lifetime and the policy defaults to off (the same variable turns it on). The HTTP reapers (`MCP_HTTP_STALE_SESSION_MS`, `MCP_HTTP_STREAM_LOST_SESSION_MS`) still reap abandoned MCP sessions, but reaping one no longer closes any debug session; a crashed client's sessions end when the idle window runs out, or sooner by an explicit close or the debuggee's exit. Step 2 of #1025. diff --git a/changelog.d/1025-ownership.changed.md b/changelog.d/1025-ownership.changed.md new file mode 100644 index 00000000..dba7a5e1 --- /dev/null +++ b/changelog.d/1025-ownership.changed.md @@ -0,0 +1 @@ +**One debugger core per `http` process: every MCP client sees the same debug sessions.** A Streamable HTTP server used to build an isolated server per MCP client session, so `list_debug_sessions` listed only the caller's own sessions and another client's id was refused with "belongs to another MCP client session" (#939). The 2026-07-28 revision of the protocol has no client sessions to key that on, so the model is now the one the spec names — the handle is the capability: a debug session is addressable by whoever holds its id, `list_debug_sessions` is the whole process's list, and several agents against one `http` process see each other's sessions (one process per agent is the isolation, as `stdio` already gives). Debug sessions therefore outlive the MCP session that created them, and a client that reconnects can list and resume or close what it left behind. The seam for real isolation is in place: a session is created in its caller's scope — the authenticated principal when the request carried one, otherwise the shared anonymous scope — and an out-of-scope id answers as unknown; with no authentication configured every caller is anonymous, which is today's single-tenant behaviour. `GET /health` now lists the process's debug sessions beside the MCP sessions, and says whether a client is attending. The ten-per-language adapter cap is per process now, as it always was on `stdio`. Step 2 of #1025. diff --git a/docs/architecture/session-tree.md b/docs/architecture/session-tree.md index c159d6d7..88c4f783 100644 --- a/docs/architecture/session-tree.md +++ b/docs/architecture/session-tree.md @@ -444,7 +444,7 @@ One row per measured fact; the "answer" column is what this design adds on top o | `detach_from_process` on an attach root with children | each child's `disconnect(terminateDebuggee: false)` resumes it undebugged (row 3) | each child keeps running (row 3) | children detached deepest-first (`released: true`), then the parent; the parent-side order with several children is measured by the M1 suite | | `restart_debugging` on a parent | the relaunch ends the previous run's processes (row 6) | same (row 6) | the previous run's children are closed and removed; `childrenClosed: n` | | grandchildren | reported on the child's connection, adopted there, `terminated` cascades bottom-up (row 7) | same (row 7) | `parentSessionId` = the child; the cap counts the whole tree | -| the HTTP client's MCP session ends (stale, stream lost, `DELETE /mcp`) | — | — | that client's server stops and closes all its sessions, trees included, as today | +| the HTTP client's MCP session ends (stale, stream lost, `DELETE /mcp`) | — | — | that client's SDK Server facade closes; its debug sessions, trees included, live on in the one core under the idle policy (`MCP_DEBUG_SESSION_IDLE_MS`; #1025 step 2) | | the parent's worker dies | — | — | every session in the tree goes to `error` with the parent's diagnostics; the adapter process is reaped by the worker's exit path as today | Per-session machinery that stays per session: the `InFlightGuard` (a child never launches or @@ -464,23 +464,29 @@ story. Ownership never becomes a tool parameter. Two guardrails: -1. In the modern era (no protocol-level MCP session) `list_debug_sessions` is the whole process's - list; the session `name` is the agent's own namespace hint. Several agents against one `http` - process see each other's sessions: that is a single-tenant statement, and one server process - per agent is the isolation, as stdio and the dev proxy already give. The legacy era's per-client - server and foreign-session refusal ([#939](https://github.com/debugmcp/mcp-debugger/issues/939)) - are that era's behaviour, not a contract. -2. Lifetime without an MCP session (today the stale reaper's trigger, `MCP_HTTP_STALE_SESSION_MS`) - becomes a **per-debug-session idle policy**: `MCP_DEBUG_SESSION_IDLE_MS`, default thirty minutes, - `0` disables. A tool call naming a session refreshes that session and its ancestors, so - actively inspecting or stepping a child cannot expire its parent and cascade-close the child. - Activity does not refresh siblings or other descendants: each keeps its own clock. An in-flight - `wait_for_stop` protects the session, all descendants it observes (including ones adopted while - waiting), and its ancestors from idle expiry for the duration of the call; their idle intervals - restart when it settles, times out or is cancelled. Closing a parent that is idle under these - rules still cascades using the initiating operation's close policy above: idle expiry of an - attach root releases all of its external targets. Explicit close and debuggee exit end a - session sooner, as today. +1. `list_debug_sessions` is the whole process's list, in every era; the session `name` is the + agent's own namespace hint. Several agents against one `http` process see each other's sessions: + that is a single-tenant statement, and one server process per agent is the isolation, as stdio + and the dev proxy already give. The legacy era's per-client server and foreign-session refusal + ([#939](https://github.com/debugmcp/mcp-debugger/issues/939)) were that era's behaviour, not a + contract, and went with the one-core-per-process change (shipped 2026-11, #1025 step 2). The + seam for real isolation is in place: a session is created in its caller's scope — the + authenticated principal when the request carried one, otherwise the shared anonymous scope — + and an out-of-scope id answers as unknown; with no authentication every caller is anonymous. +2. Lifetime without an MCP session is a **per-debug-session idle policy**: + `MCP_DEBUG_SESSION_IDLE_MS`, default thirty minutes on `http` and off on `stdio` (the pipe is + the lifetime), `0` disables; the sweep runs every `MCP_DEBUG_SESSION_IDLE_SWEEP_MS` (60 s). + Only roots have a clock: a tool call naming any session in a tree refreshes the tree's root, and + a child never expires on a clock of its own — the tree goes with its root (decided 2026-11-09, + amending the per-descendant clocks this section first proposed: a worker paused under a parent + the agent is driving must not be terminated by a timer). An in-flight `wait_for_stop` holds the + root for the duration of the call. The sweep is skipped entirely while a client is attending the + process — any open legacy SSE stream or modern `subscriptions/listen` stream — so a connected + client never loses a paused session to the clock; idle time still accrues from the last tool + call, so a vanished client's already-idle sessions end at the first unattended sweep. Expiry + cascades using the initiating operation's close policy above: idle expiry of an attach root + releases all of its external targets. Explicit close and debuggee exit end a session sooner, as + today. If real authentication ever arrives, the authenticated principal becomes the owner key with no schema change. @@ -554,9 +560,10 @@ Taken by the author of this document, marked so they can be overturned: - The child's connection lives in the parent's worker (the constraints above). - The per-tree cap defaults to 32 and is named `DEBUG_MCP_MAX_CHILD_SESSIONS`; `0` disables child-session adoption, excluding js-debug's mandatory first target. -- The idle policy knob is named `MCP_DEBUG_SESSION_IDLE_MS`. -- Activity on a descendant protects its ancestors from idle expiry; a tree-aware wait protects - the subtree it observes for the duration of the call. +- The idle policy knob is named `MCP_DEBUG_SESSION_IDLE_MS` (sweep: `MCP_DEBUG_SESSION_IDLE_SWEEP_MS`). +- Only roots have an idle clock: activity on any session in a tree refreshes its root, a child never + expires alone, a tree-aware wait holds the root for the duration of the call, and the sweep is + skipped while a client attends the process (decided with JF, 2026-11-09). - The ordering rule for several settles before one wait (parent first, then adoption order). - Ended children stay listed until closed, and leave with their parent. - Closing any session closes its subtree, including when that session is itself a child. diff --git a/docs/architecture/system-overview.md b/docs/architecture/system-overview.md index 61fda6c6..405f6960 100644 --- a/docs/architecture/system-overview.md +++ b/docs/architecture/system-overview.md @@ -298,7 +298,7 @@ This architecture enables: - **Startup Time**: ~1-2s for session initialization - **Command Latency**: <100ms for most DAP commands - **Memory Usage**: ~50MB base + ~20MB per active session -- **Concurrent Sessions**: Limited by system resources. In Streamable HTTP mode, each MCP session gets an isolated `DebugMcpServer` instance routed by `Mcp-Session-Id`, so multiple clients run independently without serialization. STDIO mode supports a single client connection. +- **Concurrent Sessions**: Limited by system resources. One `DebugMcpServer` core per process in every mode (issue #1025): in Streamable HTTP mode each MCP session is one SDK `Server` facade over it, routed by `Mcp-Session-Id`, so several clients run concurrently against the same debug sessions — the id is the capability, one process per agent is the isolation. STDIO mode supports a single client connection. ## Error Handling Strategy diff --git a/docs/development/debugging-guide.md b/docs/development/debugging-guide.md index 1310b749..5984dfb6 100644 --- a/docs/development/debugging-guide.md +++ b/docs/development/debugging-guide.md @@ -93,14 +93,15 @@ request its tool list. The outer debugger can now inspect `req.body`, step throu handler, and resume the pending request with `continue_execution`. Keep the outer debugger's connection active while the nested server is paused. -Debug sessions on a Streamable HTTP server belong to the MCP client session that created them -(each `Mcp-Session-Id` has its own `DebugMcpServer`). Drive one debug session from one client -and keep that client's MCP session open: a one-shot driver script that opens a fresh MCP session -per run cannot reach the debug session its previous run created — it is answered -`Session belongs to another MCP client session: …` — and the idle reaper closes the -abandoned one about `MCP_HTTP_STREAM_LOST_SESSION_MS` (default 2 min) after the first run's SSE -stream dropped (an SDK client holds one; a pure-POST driver falls under -`MCP_HTTP_STALE_SESSION_MS`, default 30 min), which is what finally ends its program (issue #939). +Debug sessions on a Streamable HTTP server belong to the process, not to the MCP client session +that created them (one `DebugMcpServer` core per process, one SDK `Server` facade per MCP +session; issue #1025). A one-shot driver script that opens a fresh MCP session per run reaches +the debug session its previous run created by id, and `list_debug_sessions` lists it. What ends +an abandoned one is the idle policy: a session tree no tool call has named for +`MCP_DEBUG_SESSION_IDLE_MS` (default 30 min; `0` disables) is closed, unless a client is attending +the process — while any client holds its SSE stream open, nothing expires. The abandoned MCP +sessions themselves are reaped separately (`MCP_HTTP_STREAM_LOST_SESSION_MS`, default 2 min after +the stream dropped; `MCP_HTTP_STALE_SESSION_MS`, default 30 min for a pure-POST client). Pausing a server's **proxy worker** (attach by PID to its `proxy-bootstrap.js` child; the inspector opens on 9229 unless `NODE_OPTIONS=--inspect-port=…` says otherwise) has one more @@ -542,8 +543,10 @@ Note: the HTTP command handler (`src/cli/http-command.ts`) exposes a `GET /healt # Query the built-in health endpoint (default port 3001) curl http://localhost:3001/health # Returns: { "status": "ok", "mode": "http", "listening": { "address", "port" }, "connections": N, +# "attended": bool, "listenStreams": N, # "sessions": […], -# "details": [{ "id", "openStreams", "streamLost", "idleMs", "debugSessions": [{ "id", "name", "language", "state" }] }] } +# "details": [{ "id", "openStreams", "streamLost", "idleMs" }], +# "debugSessions": [{ "id", "name", "language", "state", "parentSessionId"?, "idleMs"? }] } ``` ## Debugging Checklists diff --git a/docs/diagnostics.md b/docs/diagnostics.md index 08173990..12abfc0b 100644 --- a/docs/diagnostics.md +++ b/docs/diagnostics.md @@ -249,8 +249,10 @@ The runtime-affecting variables the server and its adapters read (the [developme | `DEBUG_MCP_LOG_LEVEL` | Server log level (`error`, `warn`, `info`, `debug`) | | `MCP_EXIT_ON_STDIN_CLOSE` | `1`/`true`: an `http` server exits when its stdin pipe closes — a parent-death signal for supervised backends | | `MCP_HTTP_STALE_SESSION_MS` | Idle time before an HTTP session that never opened an SSE stream is reaped (default `1800000`; `0` disables that path) | -| `MCP_HTTP_STREAM_LOST_SESSION_MS` | Idle time before an HTTP session whose SSE stream dropped and never returned is reaped — a crashed SDK client's debug sessions, and any paused attach target they hold, are released this soon (default `120000`; `0` falls back to the stale window; issue #658) | +| `MCP_HTTP_STREAM_LOST_SESSION_MS` | Idle time before an HTTP session whose SSE stream dropped and never returned is reaped (default `120000`; `0` falls back to the stale window; issue #658). Reaping an MCP session closes no debug session (see `MCP_DEBUG_SESSION_IDLE_MS`) | | `MCP_HTTP_STALE_SWEEP_INTERVAL_MS` | How often that reap sweep runs (default `60000`) | +| `MCP_DEBUG_SESSION_IDLE_MS` | Idle window after which a debug-session tree no tool call has named is closed, unless a client is attending the process — a crashed client's sessions, and any paused attach target they hold, are released this soon after its stream dropped (default `1800000` on `http`, `0` = off on `stdio`; `0` disables; issue #1025) | +| `MCP_DEBUG_SESSION_IDLE_SWEEP_MS` | How often that idle sweep runs (default `60000`) | | `MCP_HTTP_ALLOWED_HOSTS` | Comma-separated `Host`/`Origin` hostnames the `http` endpoint accepts besides the loopback trio (same as repeating `--allowed-host`); an unusable entry stops startup with the entry named | | `DAP_TRACE` / `DAP_TRACE_FILE` | Enable per-session DAP frame capture / choose its file | | `DEBUG_MCP_NO_REDACT` | Disable secret redaction in captured output (diagnosis only) | diff --git a/docs/docker-support.md b/docs/docker-support.md index 3ccb5d48..20823379 100644 --- a/docs/docker-support.md +++ b/docs/docker-support.md @@ -258,20 +258,26 @@ may reach it. All are passed through with `-e`: (or omit the variable) for detached network-mode containers. It is unrelated to the `stdio`-mode exit policy described under [Troubleshooting](#common-docker-issues), which is always on and needs no variable. +- **`MCP_DEBUG_SESSION_IDLE_MS`** (`src/session/idle-policy.ts`) -- the idle window after which + a debug-session tree that no tool call has named is closed, releasing any ptrace claim on the + target, unless a client is attending the process (an open SSE stream). Defaults to 30 minutes + (1800000) in `http` mode and off in `stdio` mode, where the pipe is the lifetime; `0` disables; + invalid values log a warning and fall back to the default. `MCP_DEBUG_SESSION_IDLE_SWEEP_MS` + (default 60000) is the sweep cadence. A short-lived diagnostic container wants a much tighter + window -- see the [Kubernetes debugging recipe](kubernetes.md), which sets it to 5 minutes + (issue #1025). - **`MCP_HTTP_STALE_SESSION_MS`** (`src/cli/http-command.ts`) -- idle window before an HTTP - MCP session with no open SSE stream is reaped, closing its debug sessions and releasing - any ptrace claim on the target. Defaults to 30 minutes (1800000); `0` disables that path; - invalid values log a warning and fall back to the default. Read only by `http` mode. - A short-lived diagnostic container wants a - much tighter window -- see the - [Kubernetes debugging recipe](kubernetes.md), which sets it to 5 minutes. -- **`MCP_HTTP_STREAM_LOST_SESSION_MS`** -- the short window for a session whose SSE stream + MCP session with no open SSE stream is reaped. Defaults to 30 minutes (1800000); `0` disables + that path; invalid values log a warning and fall back to the default. Read only by `http` + mode. Reaping an MCP session closes no debug session: those belong to the process and answer + to `MCP_DEBUG_SESSION_IDLE_MS`. +- **`MCP_HTTP_STREAM_LOST_SESSION_MS`** -- the short window for an MCP session whose SSE stream dropped and never came back (default 2 minutes; `0` falls back to the stale window). An MCP SDK client keeps a GET stream open, so when it crashes or exits without `DELETE /mcp` - the server sees the socket close and releases its debug sessions -- including a paused - attach target -- within this window plus one sweep, instead of holding them for the full - stale window (issue #658). Only a client that never opened a stream waits out - `MCP_HTTP_STALE_SESSION_MS`. `GET /health` lists what each HTTP session is holding. + the server sees the socket close and forgets the connection within this window plus one + sweep (issue #658); the moment that stream closes the process is also no longer attended, so + the idle policy can end the debug sessions it left behind. `GET /health` lists the MCP + sessions, the debug sessions and whether anyone is attending. - **`MCP_HTTP_ALLOWED_HOSTS`** (`src/cli/host-allowlist.ts`) -- comma-separated `Host` header values to accept in addition to the loopback trio (`localhost`, `127.0.0.1`, `[::1]`); the same list as repeating `--allowed-host`. Port-agnostic and case-insensitive; IPv6 literals in diff --git a/docs/jit-diagnostics/README.md b/docs/jit-diagnostics/README.md index 39ed8680..2caffab9 100644 --- a/docs/jit-diagnostics/README.md +++ b/docs/jit-diagnostics/README.md @@ -128,7 +128,7 @@ kubectl port-forward "pod/$POD" 3001:3001 & Register the endpoint with your agent (e.g. `claude mcp add-json sick-pod '{"type":"http","url":"http://127.0.0.1:3001/mcp"}'`). -The HTTP server reaps MCP sessions abandoned by a crashed client, closing their debug sessions and releasing any ptrace claim on the target: a client whose SSE stream dropped and never returned (any MCP SDK client that crashed or exited without `DELETE`) is reaped after 2 minutes idle (`MCP_HTTP_STREAM_LOST_SESSION_MS`), one that never opened a stream after 30 (`MCP_HTTP_STALE_SESSION_MS`). For a short-lived diagnostic sidecar a tighter stale window is safer — add `--env=MCP_HTTP_STALE_SESSION_MS=300000` (5 minutes; `0` disables) to the `kubectl debug` command. `GET /health` on the sidecar lists what each MCP session is holding. +The HTTP server closes debug sessions abandoned by a crashed client, releasing any ptrace claim on the target, through the per-debug-session idle policy: a session tree no tool call has named for 30 minutes is closed unless a client is attending the process (`MCP_DEBUG_SESSION_IDLE_MS`; `0` disables). For a short-lived diagnostic sidecar a tighter window is safer — add `--env=MCP_DEBUG_SESSION_IDLE_MS=300000` (5 minutes) to the `kubectl debug` command. The abandoned MCP sessions themselves are reaped separately: a client whose SSE stream dropped and never returned (any MCP SDK client that crashed or exited without `DELETE`) is reaped after 2 minutes idle (`MCP_HTTP_STREAM_LOST_SESSION_MS`), one that never opened a stream after 30 (`MCP_HTTP_STALE_SESSION_MS`). For a short-lived diagnostic sidecar a tighter stale window is safer — add `--env=MCP_HTTP_STALE_SESSION_MS=300000` (5 minutes; `0` disables) to the `kubectl debug` command. `GET /health` on the sidecar lists what each MCP session is holding. Facts that will save you an afternoon (all observed, not assumed): diff --git a/docs/kubernetes.md b/docs/kubernetes.md index ac033a5b..a7a15ec3 100644 --- a/docs/kubernetes.md +++ b/docs/kubernetes.md @@ -122,7 +122,7 @@ binary. Send the debugger to it: POD=$(kubectl get pods -l app=cpp-demo -o jsonpath='{.items[0].metadata.name}') kubectl debug "$POD" --image=debugmcp/mcp-debugger:latest \ --target=app --profile=general --container=debugger \ - --env=MCP_HTTP_STALE_SESSION_MS=300000 \ + --env=MCP_DEBUG_SESSION_IDLE_MS=300000 --env=MCP_HTTP_STALE_SESSION_MS=300000 \ -- /app/entry.sh http -p 3001 kubectl port-forward "pod/$POD" 3001:3001 & ``` @@ -144,11 +144,14 @@ The flags that matter — each one is load-bearing: pass `-- /app/entry.sh http -p 3001` — bare `-- http -p 3001` fails with `exec: "http": executable file not found`. (Git Bash on Windows mangles `/app/...` — prefix the command with `MSYS_NO_PATHCONV=1`.) -- **`MCP_HTTP_STALE_SESSION_MS=300000`** tightens the reaper that cleans up MCP - sessions abandoned by a client that never opened an SSE stream from 30 min - to 5 — releasing the ptrace claim on your pod sooner. (A crashed SDK client, - whose stream the server saw close, is reaped after 2 min regardless — - `MCP_HTTP_STREAM_LOST_SESSION_MS`.) +- **`MCP_DEBUG_SESSION_IDLE_MS=300000`** tightens the idle policy that closes a + debug session nobody has named from 30 min to 5 — releasing the ptrace claim + on your pod sooner once the client is gone (while a client holds its SSE + stream open, nothing expires). **`MCP_HTTP_STALE_SESSION_MS=300000`** tightens + the separate reaper for abandoned MCP sessions the same way. (A crashed SDK + client's MCP session, whose stream the server saw close, is reaped after + 2 min regardless — `MCP_HTTP_STREAM_LOST_SESSION_MS`; its debug sessions then + answer to the idle policy.) - Ephemeral containers **cannot be removed or restarted** — each retry needs a fresh `--container` name; only a pod restart clears them. @@ -188,7 +191,8 @@ though `--profile=general` avoids a deprecation warning): ```bash kubectl debug "$POD" --image=debugmcp/mcp-debugger:latest --profile=general \ - --container=debugger --env=MCP_HTTP_STALE_SESSION_MS=300000 -- /app/entry.sh http -p 3001 + --container=debugger --env=MCP_DEBUG_SESSION_IDLE_MS=300000 --env=MCP_HTTP_STALE_SESSION_MS=300000 \ + -- /app/entry.sh http -p 3001 ``` Then `attach_to_process {host: "127.0.0.1", port: 5678, ...}` from a session on @@ -220,8 +224,9 @@ the MCP endpoint, and the containerized server attaches to python, ruby logpoints (`set_breakpoint` with `logMessage`): the pod keeps running at full speed while values stream to `get_output`. - An abandoned attach no longer leaves an orphaned tracer (#337): teardown - kills the adapter's process group, and the sidecar's HTTP reaper cleans up - crashed-client sessions after the stale window. Suspect a leftover anyway? + kills the adapter's process group, and the sidecar's idle policy closes a + crashed client's debug sessions once nobody attends the process + (`MCP_DEBUG_SESSION_IDLE_MS`). Suspect a leftover anyway? `grep TracerPid /proc/1/status` from the sidecar. ## Distroless and minimal images diff --git a/docs/tool-reference.md b/docs/tool-reference.md index daa4ead0..f1280de4 100644 --- a/docs/tool-reference.md +++ b/docs/tool-reference.md @@ -87,14 +87,14 @@ Creates a new debugging session. **Notes:** - Session IDs are UUIDs in the format `xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx` - Sessions start in `"created"` state -- **Streamable HTTP: debug sessions are per MCP client session.** Each HTTP client (each `Mcp-Session-Id`) gets its own server, so a debug session is visible only to the client that created it; `list_debug_sessions` lists that client's sessions alone. A tool call from another client naming the id answers `success: false` with `error: "Session belongs to another MCP client session: …"` and says how it ends: only its client can close it or end its MCP session with an explicit `DELETE /mcp` (an SDK client's `close()` is not a DELETE — a plain disconnect leaves it to the idle reaper: `MCP_HTTP_STREAM_LOST_SESSION_MS` for a client that held an SSE stream, `MCP_HTTP_STALE_SESSION_MS` for one that never did; both `0` means nothing else ends it). Drive one debug session from one client and keep that client's MCP session open; a one-shot script that opens a fresh MCP session per call cannot reach the sessions its previous run created (issue #939). stdio has one client per process +- **One debugger core per process; the id is the capability (issue #1025).** On Streamable HTTP every MCP client of a process sees the same debug sessions: `list_debug_sessions` is the whole process's list, and any client holding an id can drive or close that session — run one `http` process per agent where agents must not see each other's work. A debug session outlives the MCP session that created it, so a one-shot script that opens a fresh MCP session per call reaches the sessions its previous run created, and a client that reconnects can resume or close what it left behind. What ends an abandoned session is the **idle policy**: a session tree that no tool call has named for `MCP_DEBUG_SESSION_IDLE_MS` (default 30 minutes on `http`, off on `stdio`; `0` disables) is closed through the normal close cascade, unless a client is attending the process — while any client holds its SSE stream open, nothing expires; an in-flight `wait_for_stop` holds its tree; children never expire on a clock of their own. Explicit close and debuggee exit end a session sooner, as always. With authentication in front of `/mcp`, sessions are scoped to the principal that created them and an out-of-scope id answers as unknown; with none, every caller is in the one shared scope - When `port`, `processId` or an `adapterConfig` URI/info-file key is provided in `create_debug_session`, the server performs an inline attach (creating the session and immediately attaching). The session exists before the addressing gate can refuse the form (a `port` on C/C++, say), so such a refusal answers `success: false` with a usable `sessionId`. The response then mirrors `attach_to_process`: alongside `sessionId` it carries `state`, the attach `data` payload, an optional `warning`, and — when a requested post-attach pause has not landed yet — `pending: true`, with `message` saying so (`…; post-attach pause pending — the target stops when it next executes code; call wait_for_stop to block until it does (pass stopOnEntry: false to attach without pausing)`) --- ### list_debug_sessions -Lists all active debugging sessions — in Streamable HTTP mode, the ones this MCP client session created (see the note under `create_debug_session`). +Lists all active debugging sessions — the whole process's list, whichever MCP client created them (see the note under `create_debug_session`). **Parameters:** None (empty object `{}`) @@ -169,7 +169,7 @@ A session with child sessions, a child session or an attach root says what the c **Notes:** - Sessions may close automatically on errors -- Closing a non-existent session returns `success: false` (`message: "Failed to close debug session: "`); in Streamable HTTP mode a session another MCP client created answers `error: "Session belongs to another MCP client session: …"` instead, and stays open (issue #939) +- Closing a non-existent session returns `success: false` (`message: "Failed to close debug session: "`); a session another MCP client of the same process created closes like any other — the id is the capability (issue #1025) - On a launch session the debuggee is terminated. Adapters that need a DAP `terminate` ahead of the disconnect to do so (the Dart and Flutter SDK adapters) receive one first (#885); an attached process is detached, not terminated @@ -1439,7 +1439,7 @@ Each session also exposes its captured output as an MCP resource: - **URI:** `debug://sessions/{sessionId}/output` (`text/plain`) — the verbatim console transcript (all categories interleaved in arrival order). - **`resources/list`** enumerates one output resource per session; the list changes on session create/close (`notifications/resources/list_changed`). - **`resources/subscribe`** to a session's URI to receive `notifications/resources/updated` pings as output arrives. Pings are coalesced (~150 ms), so notification volume is independent of how fast the debuggee prints — on a ping, re-read the resource or call `get_output` with your cursor. -- Subscriptions are tracked per server instance and cleaned up when the session closes. +- Subscriptions are tracked per MCP connection and cleaned up when the connection closes or the session is removed; the coalesced pings are fanned out to every connection that subscribed. After a launch or attach creates its run directory, `resources/list` also includes `debug://sessions/{sessionId}/proxy-log`. Reading it returns at most the final 64 KiB, sanitized and trimmed to 80 lines. It is a point-in-time diagnostic resource and is intentionally not subscribable; output remains the only resource that emits update notifications. @@ -1821,7 +1821,7 @@ Session-lifecycle failures (unknown/terminated session, proxy not running) are a ``` ### Common Error Scenarios -1. **Session not found**: Occurs when a session terminates unexpectedly. In Streamable HTTP mode an id that exists under *another* MCP client session is not reported as not found: every tool answers `error: "Session belongs to another MCP client session: …"` (issue #939) +1. **Session not found**: Occurs when a session terminates unexpectedly, or when the idle policy closed it (`MCP_DEBUG_SESSION_IDLE_MS`, see `create_debug_session`). An id created by another MCP client of the same process is found like any other; one outside the caller's scope (only with authentication in front of `http`) answers exactly as an unknown id (issue #1025) 2. **Invalid language**: Language must be one of the supported languages (discovered dynamically from installed adapters) 3. **File not found**: When setting breakpoints in non-existent files 4. **Invalid scope**: When passing wrong variablesReference to get_variables diff --git a/examples/kubernetes/debug-sidecar.sh b/examples/kubernetes/debug-sidecar.sh index dcfe78d4..7bc2d9a9 100644 --- a/examples/kubernetes/debug-sidecar.sh +++ b/examples/kubernetes/debug-sidecar.sh @@ -22,7 +22,7 @@ POD=$(kubectl -n "$NS" get pods -l "$SELECTOR" -o jsonpath='{.items[0].metadata. echo "Injecting ephemeral container '$NAME' into pod/$POD (target container: app)..." kubectl -n "$NS" debug "$POD" --image=debugmcp/mcp-debugger:latest \ --target=app --profile=general --container="$NAME" \ - --env=MCP_HTTP_STALE_SESSION_MS=300000 \ + --env=MCP_DEBUG_SESSION_IDLE_MS=300000 --env=MCP_HTTP_STALE_SESSION_MS=300000 \ -- /app/entry.sh http -p 3001 echo "Register with your agent: claude mcp add-json k8s-sidecar '{\"type\":\"http\",\"url\":\"http://127.0.0.1:3001/mcp\"}'" diff --git a/packages/mcp-debugger/README.md b/packages/mcp-debugger/README.md index 5ec41d2b..7e475d78 100644 --- a/packages/mcp-debugger/README.md +++ b/packages/mcp-debugger/README.md @@ -100,7 +100,9 @@ All language adapters are bundled into the CLI package. No separate installation | `DART_SDK` | Dart SDK root (or its `bin/dart`) when `dart` is not on PATH; `DART_PATH` is an alias | | `FLUTTER_ROOT` | Flutter SDK root when `flutter` is not on PATH — its bundled Dart is used when no standalone SDK is found; `FLUTTER_PATH` is an alias | | `MCP_HTTP_STALE_SESSION_MS` | HTTP mode: reap a crash-abandoned MCP session that never opened an SSE stream after this idle time (default 30 min) | -| `MCP_HTTP_STREAM_LOST_SESSION_MS` | HTTP mode: reap a session whose SSE stream dropped and never returned after this idle time (default 2 min) — releases a dead client's paused attach target | +| `MCP_HTTP_STREAM_LOST_SESSION_MS` | HTTP mode: reap a crash-abandoned MCP session whose SSE stream dropped and never returned after this idle time (default 2 min) | +| `MCP_DEBUG_SESSION_IDLE_MS` | Close a debug-session tree no tool call has named for this long, unless a client is attending the process (default 30 min in HTTP mode, off in stdio mode; `0` disables) — releases a dead client's paused attach target | +| `MCP_DEBUG_SESSION_IDLE_SWEEP_MS` | How often that idle sweep runs (default 60 s) | | `MCP_HTTP_ALLOWED_HOSTS` | HTTP mode: comma-separated `Host` (and browser `Origin`) hostnames to accept besides `localhost`, `127.0.0.1`, `[::1]` — only when another access control fronts the server; same as repeating `--allowed-host` | | `MCP_HTTP_BIND` | HTTP mode: IP address to listen on (default `127.0.0.1`, loopback only); same as `--bind`. The Docker image sets `0.0.0.0` so a published port works. An empty value refuses to start | diff --git a/scripts/k8s-smoke.mjs b/scripts/k8s-smoke.mjs index 60fc85d1..2ffeeaec 100644 --- a/scripts/k8s-smoke.mjs +++ b/scripts/k8s-smoke.mjs @@ -433,8 +433,9 @@ class McpHttpClient extends McpClientBase { async dispose() { if (!this.sessionId) return; try { - // Best-effort session teardown so the sidecar releases its ptrace claim - // promptly instead of waiting for the stale-session reaper. + // Best-effort MCP-session teardown. The debug sessions are the process's + // (issue #1025) and are closed by the test's own close_debug_session + // calls or, failing that, by the idle policy once nobody attends. await fetch(this.url, { method: 'DELETE', headers: this.headers() }); transcribe('note', 'DELETE /mcp sent'); } catch (err) { diff --git a/src/cli/http-command.ts b/src/cli/http-command.ts index 8f51b8ee..df69d6e7 100644 --- a/src/cli/http-command.ts +++ b/src/cli/http-command.ts @@ -3,7 +3,7 @@ import type { Express, Request, Response, NextFunction } from 'express'; import express from 'express'; import { randomUUID } from 'crypto'; import { IncomingMessage, ServerResponse } from 'http'; -import { isInitializeRequest, ProtocolErrorCode, type Server } from '@modelcontextprotocol/server'; +import { isInitializeRequest, ProtocolErrorCode } from '@modelcontextprotocol/server'; import { NodeStreamableHTTPServerTransport } from '@modelcontextprotocol/node'; import { DebugMcpServer, type DebugMcpServerOptions } from '../server.js'; import { createProtocolServer } from '../server/protocol-facade.js'; @@ -45,11 +45,14 @@ export interface HttpCommandDependencies { proc?: ProcessLike; } +/** + * One MCP (protocol-level) session of the 2025 era: a connection, not an + * owner. Its SDK Server facade lives in the transport's onmessage closure and + * leaves with it; the debug sessions it created belong to the one core + * (issue #1025, step 2). + */ interface SessionData { transport: NodeStreamableHTTPServerTransport; - server: DebugMcpServer; - /** The SDK Server facade connected to this session's transport (issue #1025, step 2). */ - facade: Server; /** Timestamp of the last routed request (or stream close) for this session. */ lastActivity: number; /** Open SSE (GET) streams; a session with a live stream is never idle. */ @@ -67,6 +70,12 @@ const JSON_BODY_LIMIT = '10mb'; /** Idle window before a streamless HTTP session is reaped (issue #337). */ const DEFAULT_STALE_SESSION_MS = 30 * 60 * 1000; +/** + * The per-debug-session idle policy's default on http (issue #1025, step 2): + * a debug-session tree no tool call has named for this long is closed, unless + * a client is attending the process. MCP_DEBUG_SESSION_IDLE_MS overrides it. + */ +const DEFAULT_DEBUG_SESSION_IDLE_MS = 30 * 60 * 1000; /** * Idle window before a session whose SSE stream dropped and never returned is * reaped (issue #658). The SDK client re-opens a lost stream within seconds @@ -192,30 +201,44 @@ export function createHttpApp( const httpSessions = new Map(); - // Reap crash-abandoned sessions (issue #337): a client that dies without + // One core for the process (issue #1025, step 2): the dependency container, + // the SessionManager and every debug session, shared by every MCP session's + // facade. Built here, after the refusal gates above, so a refused flag never + // constructs one; handleHttpCommand starts it before listening. + const core = serverFactory({ + logLevel: options.logLevel, + logFile: options.logFile, + debugSessionIdle: { defaultMs: DEFAULT_DEBUG_SESSION_IDLE_MS }, + }); + // While any client holds an open SSE stream the process is attended and the + // idle policy closes nothing; the modern era's listen streams join this + // count when they are served. + core.setAttendedSupplier(() => { + for (const session of httpSessions.values()) { + if (session.openStreams > 0) { + return true; + } + } + return false; + }); + + // Reap crash-abandoned MCP sessions (issue #337): a client that dies without // DELETE leaves its transport registered forever — transport.onclose only - // fires on explicit close — so its DebugMcpServer and every proxy chain - // (including lldb-server's ptrace claim on an attach target, or a paused - // attach target itself) stay alive, invisible to the reconnecting client's - // fresh session. Sessions holding a live SSE stream are never reaped. A - // session whose stream dropped and never came back is reaped after + // fires on explicit close — so its SSE stream bookkeeping and its SDK Server + // facade would stay alive. Sessions holding a live SSE stream are never + // reaped. A session whose stream dropped and never came back is reaped after // MCP_HTTP_STREAM_LOST_SESSION_MS (default 2 min; issue #658) — the SDK // client keeps a GET stream open, so its death is observed the moment the // socket closes and need not wait out the long window. A pure-POST client, // whose liveness is unobservable, is reaped only after // MCP_HTTP_STALE_SESSION_MS (default 30 min). 0 disables either path. Both - // close the transport through the normal onclose path, which stops its - // server and debug sessions. + // close the transport through the normal onclose path. The debug sessions + // such a client created are the core's and answer to the per-debug-session + // idle policy (MCP_DEBUG_SESSION_IDLE_MS), not to this sweep (issue #1025). const staleSessionMs = parseStaleSessionMs(proc.env.MCP_HTTP_STALE_SESSION_MS, logger); const streamLostSessionMs = parseStreamLostSessionMs(proc.env.MCP_HTTP_STREAM_LOST_SESSION_MS, logger); const staleSweepIntervalMs = parseSweepIntervalMs(proc.env.MCP_HTTP_STALE_SWEEP_INTERVAL_MS, logger); - // A debug session id that another HTTP session's server holds answers with - // whose it is (issue #939): every tool call runs against the calling - // client's own server, which cannot see the others, so each new server is - // handed this lookup (in handleMcpRequest below). It walks the live map at - // call time — nothing to keep in sync, and the asking server, which has - // already missed, is harmlessly among the entries. let staleSweepTimer: NodeJS.Timeout | undefined; if (staleSessionMs > 0 || streamLostSessionMs > 0) { const windows = { staleMs: staleSessionMs, streamLostMs: streamLostSessionMs }; @@ -227,15 +250,12 @@ export function createHttpApp( continue; } const idleSeconds = Math.round((now - session.lastActivity) / 1000); - const held = session.server.sessionManager.getAllSessions() - .map((debugSession) => `${debugSession.id} (${debugSession.language}, ${debugSession.state})`); - const holding = held.length > 0 ? `; holding debug session(s) ${held.join(', ')}` : ''; if (verdict === 'stream-lost') { logger.warn( - `Reaping HTTP session ${sid}: its SSE stream closed ${idleSeconds}s ago and the client neither reconnected nor sent a request (MCP_HTTP_STREAM_LOST_SESSION_MS=${streamLostSessionMs})${holding}.` + `Reaping HTTP session ${sid}: its SSE stream closed ${idleSeconds}s ago and the client neither reconnected nor sent a request (MCP_HTTP_STREAM_LOST_SESSION_MS=${streamLostSessionMs}).` ); } else { - logger.warn(`Reaping stale HTTP session ${sid} (idle ${idleSeconds}s, no open streams)${holding}.`); + logger.warn(`Reaping stale HTTP session ${sid} (idle ${idleSeconds}s, no open streams).`); } try { void session.transport.close(); @@ -283,6 +303,9 @@ export function createHttpApp( const sessionId = Array.isArray(sessionIdHeader) ? sessionIdHeader[0] : sessionIdHeader; let transport: NodeStreamableHTTPServerTransport; + // Set when this request opens a new MCP session, so a handshake the SDK + // refuses does not leave its transport and facade behind (issue #1025). + let opening: NodeStreamableHTTPServerTransport | undefined; if (sessionId && httpSessions.has(sessionId)) { // Existing session — route to its transport @@ -303,15 +326,9 @@ export function createHttpApp( } transport = session.transport; } else if (!sessionId && req.method === 'POST' && isInitializeRequest(req.body)) { - // New session — spin up an isolated DebugMcpServer + transport - const newDebugServer = serverFactory({ - logLevel: options.logLevel, - logFile: options.logFile, - }); - await newDebugServer.start(); - // The SDK Server this session speaks through: a legacy-era facade over - // the core (issue #1025, step 2). - const facade = protocolServerFactory(newDebugServer, { era: 'legacy', deliver: 'instance' }); + // New MCP session: a legacy-era SDK Server facade over the one core + // (issue #1025, step 2), connected to its own sessionful transport. + const facade = protocolServerFactory(core, { era: 'legacy', deliver: 'instance' }); // Forward declarations so the closures below can refer to the transport. // The SDK assigns its own internal sessionId before invoking onsessioninitialized. @@ -323,8 +340,6 @@ export function createHttpApp( if (createdTransport) { httpSessions.set(sid, { transport: createdTransport, - server: newDebugServer, - facade, lastActivity: Date.now(), openStreams: 0, hadStream: false, @@ -339,16 +354,13 @@ export function createHttpApp( // is already in place and then runs its own teardown, which is what // reaches server.onclose and detaches the facade's notification sink. // Assigning after connect() would replace that wrapper (issue #1025). + // The debug sessions this client created live on in the core under the + // idle policy; the MCP session was only a connection. newTransport.onclose = () => { const sid = newTransport.sessionId; if (!sid) return; - const session = httpSessions.get(sid); - if (!session) return; - httpSessions.delete(sid); + if (!httpSessions.delete(sid)) return; logger.info(`HTTP session closed: ${sid}`); - session.server.stop().catch((err) => { - logger.error(`Error stopping debug server for session ${sid}:`, err); - }); }; newTransport.onerror = (error: Error) => { @@ -358,6 +370,7 @@ export function createHttpApp( await facade.connect(newTransport); transport = newTransport; + opening = newTransport; } else if (sessionId) { logger.warn('Rejecting MCP request: unknown session ID', { method: req.method }); res.status(404).json(jsonRpcErrorBody(-32001, 'Session not found')); @@ -378,6 +391,15 @@ export function createHttpApp( } await transport.handleRequest(req as IncomingMessage, res as ServerResponse, req.body); + if (opening && !opening.sessionId) { + // The handshake did not complete (the SDK refused it): nothing registered + // this transport, so nothing would ever close it. Closing it runs the + // facade's onclose chain and detaches its notification sink. + logger.debug('Closing the transport of an initialize that did not complete'); + await opening.close().catch((err: unknown) => { + logger.debug('Error closing an uninitialized transport', { error: err }); + }); + } } catch (error) { logger.error('Error handling MCP request', { error }); if (!res.headersSent) { @@ -392,29 +414,37 @@ export function createHttpApp( app.get('/mcp', handleMcpRequest); app.delete('/mcp', handleMcpRequest); - // /health names what each HTTP session holds (issue #658): an orphaned - // session's debug sessions are invisible to every other MCP client, so - // this is the operator's only view of a paused attach target short of ps. + // /health names every MCP session (a connection) and every debug session of + // the process (issues #658, #1025): the operator's view of a paused attach + // target short of ps, and of whether a client is attending. app.get('/health', (_req: Request, res: Response) => { const now = Date.now(); + const sessions = Array.from(httpSessions, ([id, session]) => ({ + id, + openStreams: session.openStreams, + streamLost: session.hadStream && session.openStreams === 0, + idleMs: now - session.lastActivity, + })); res.json({ status: 'ok', mode: 'http', listening: { ...listening }, connections: httpSessions.size, + attended: sessions.some((session) => session.openStreams > 0), + listenStreams: 0, sessions: Array.from(httpSessions.keys()), - details: Array.from(httpSessions, ([id, session]) => ({ - id, - openStreams: session.openStreams, - streamLost: session.hadStream && session.openStreams === 0, - idleMs: now - session.lastActivity, - debugSessions: session.server.sessionManager.getAllSessions().map((debugSession) => ({ + details: sessions, + debugSessions: core.sessionManager.getAllSessions().map((debugSession) => { + const idleMs = core.sessionManager.idleMsOf(debugSession.id, now); + return { id: debugSession.id, name: debugSession.name, language: debugSession.language, state: debugSession.state, - })), - })), + ...(debugSession.parentSessionId ? { parentSessionId: debugSession.parentSessionId } : {}), + ...(idleMs !== undefined ? { idleMs } : {}), + }; + }), }); }); @@ -439,6 +469,8 @@ export function createHttpApp( // eslint-disable-next-line @typescript-eslint/no-explicit-any (app as any).httpSessions = httpSessions; // eslint-disable-next-line @typescript-eslint/no-explicit-any + (app as any).core = core; + // eslint-disable-next-line @typescript-eslint/no-explicit-any (app as any).bindAddress = bind; // eslint-disable-next-line @typescript-eslint/no-explicit-any (app as any).listening = listening; @@ -495,6 +527,11 @@ export async function handleHttpCommand( // eslint-disable-next-line @typescript-eslint/no-explicit-any const httpSessions = (app as any).httpSessions as Map; // eslint-disable-next-line @typescript-eslint/no-explicit-any + const core = (app as any).core as DebugMcpServer; + // The one core (issue #1025, step 2): started once, before anything can + // reach it; its idle sweep is armed here. + await core.start(); + // eslint-disable-next-line @typescript-eslint/no-explicit-any const bind = (app as any).bindAddress as ResolvedBindAddress; // eslint-disable-next-line @typescript-eslint/no-explicit-any const listening = (app as any).listening as ListeningEndpoint; @@ -547,20 +584,21 @@ export async function handleHttpCommand( }, 15000); hardExit.unref?.(); - // Close every active transport and stop its DebugMcpServer, bounded - // so one wedged session cannot stall the whole shutdown. + // Close every active transport, then stop the one core (every debug + // session with it), bounded so one wedged proxy cannot stall the whole + // shutdown. Transports first: no request lands on a core that is closing. const stopWork = (async () => { - for (const { transport, server: debugServer } of httpSessions.values()) { + for (const { transport } of httpSessions.values()) { try { await transport.close(); } catch (err) { logger.error('Error closing transport during shutdown', { error: err }); } - try { - await debugServer.stop(); - } catch (err) { - logger.error('Error stopping debug server during shutdown', { error: err }); - } + } + try { + await core.stop(); + } catch (err) { + logger.error('Error stopping debug server during shutdown', { error: err }); } })(); const guard = new Promise((resolve) => { diff --git a/src/cli/stdio-command.ts b/src/cli/stdio-command.ts index 9b34e732..88fe2d04 100644 --- a/src/cli/stdio-command.ts +++ b/src/cli/stdio-command.ts @@ -1,14 +1,11 @@ import type { Logger as WinstonLoggerType } from 'winston'; import { StdioServerTransport } from '@modelcontextprotocol/server/stdio'; -import { DebugMcpServer } from '../server.js'; +import { DebugMcpServer, type DebugMcpServerOptions } from '../server.js'; import { createProtocolServer } from '../server/protocol-facade.js'; import type { StdioOptions } from './setup.js'; import type { ProcessLike } from '../interfaces/process-interfaces.js'; -export interface ServerFactoryOptions { - logLevel?: string; - logFile?: string; -} +export type ServerFactoryOptions = DebugMcpServerOptions; export interface StdioCommandDependencies { logger: WinstonLoggerType; @@ -39,7 +36,10 @@ export async function handleStdioCommand( const debugMcpServer = serverFactory({ logLevel: options.logLevel, - logFile: options.logFile + logFile: options.logFile, + // On stdio the pipe is the lifetime: the per-debug-session idle policy + // defaults to off here (MCP_DEBUG_SESSION_IDLE_MS still turns it on; issue #1025). + debugSessionIdle: { defaultMs: 0 } }); // Create stdio transport diff --git a/src/server.ts b/src/server.ts index edcad552..636d2779 100644 --- a/src/server.ts +++ b/src/server.ts @@ -878,8 +878,8 @@ export class DebugMcpServer implements ToolContext { this.sessionManager.removeListener('child-session-adopted', this.handleChildSessionAdopted); this.sessionManager.removeListener('session-removed', this.handleSessionRemoved); this.logger.info('Debug MCP Server stopped'); - // Last: detach this server's logger from the shared file transport so - // per-session servers in HTTP mode don't accumulate on it (issue #404). + // Last: detach this core's logger from the shared file transport (issue + // #404; one core per process now, so this runs once, at shutdown). // After this line the logger no longer writes to the shared file. this.disposeLogger?.(); } diff --git a/tests/e2e/mcp-server-smoke-http-stale-reap.test.ts b/tests/e2e/mcp-server-smoke-http-stale-reap.test.ts index 29d9a9d7..ce17de7d 100644 --- a/tests/e2e/mcp-server-smoke-http-stale-reap.test.ts +++ b/tests/e2e/mcp-server-smoke-http-stale-reap.test.ts @@ -115,7 +115,11 @@ describe('MCP Server E2E HTTP stale-session reap (issue #502)', () => { env: { ...process.env, MCP_HTTP_STALE_SESSION_MS: '2000', - MCP_HTTP_STALE_SWEEP_INTERVAL_MS: '1000' + MCP_HTTP_STALE_SWEEP_INTERVAL_MS: '1000', + // The debug session is the core's (issue #1025): the MCP-session reap + // above no longer ends it; the per-debug-session idle policy does. + MCP_DEBUG_SESSION_IDLE_MS: '2000', + MCP_DEBUG_SESSION_IDLE_SWEEP_MS: '1000' } }); const serverPid = serverProcess.pid!; @@ -173,7 +177,7 @@ describe('MCP Server E2E HTTP stale-session reap (issue #502)', () => { await mcpClient.close(); mcpClient = null; - // The stale sweep (idle > 2s, ticked every 1s) must reap the session… + // The stale sweep (idle > 2s, ticked every 1s) must reap the MCP session… const reaped = await pollUntil(async () => { try { return readFileSync(logFile, 'utf8').includes('Reaping stale HTTP session') @@ -185,6 +189,19 @@ describe('MCP Server E2E HTTP stale-session reap (issue #502)', () => { }, 20000, 250); expect(reaped).toBe(true); + // …the idle policy must close the debug session once nobody attends the + // process (the client's SSE stream closed with it; issue #1025)… + const idleClosed = await pollUntil(async () => { + try { + return readFileSync(logFile, 'utf8').includes('Closing idle debug session') + ? true + : undefined; + } catch { + return undefined; + } + }, 20000, 250); + expect(idleClosed).toBe(true); + // …and the worker OS process must actually die. const workerGone = await pollUntil( async () => (pidAlive(workerPid!) ? undefined : true), diff --git a/tests/integration/cli/http-foreign-session.test.ts b/tests/integration/cli/http-foreign-session.test.ts deleted file mode 100644 index 286f32a4..00000000 --- a/tests/integration/cli/http-foreign-session.test.ts +++ /dev/null @@ -1,115 +0,0 @@ -/** - * Streamable HTTP: a debug session is visible only to the MCP client session - * that created it — one DebugMcpServer per client (issues #337, #658). A second - * client naming that id used to get the same `Session not found` an invalid id - * gets, while the program kept running under the first client, unreachable - * (issue #939). The answer now says whose it is. Real server, two SDK clients, - * the mock adapter — no toolchain. - */ -import { afterEach, describe, expect, it } from 'vitest'; -import { spawn, type ChildProcess } from 'node:child_process'; -import net, { type AddressInfo } from 'node:net'; -import path from 'node:path'; -import { fileURLToPath } from 'node:url'; -import { Client, StreamableHTTPClientTransport } from '@modelcontextprotocol/client'; -import { serverEnvironment } from '../../test-utils/helpers/mcp-stdio-client.js'; -import { waitForHealthEndpoint } from '../../e2e/smoke-test-utils.js'; - -const root = fileURLToPath(new URL('../../../', import.meta.url)); -const entry = path.join(root, 'dist/index.js'); - -interface Answer { - success: boolean; - error?: string; - message?: string; - sessionId?: string; - sessions?: Array<{ id: string }>; -} - -let server: ChildProcess | undefined; -let serverOutput = ''; -const clients: Client[] = []; - -function freePort(): Promise { - return new Promise((resolve, reject) => { - const probe = net.createServer(); - probe.on('error', reject); - probe.listen(0, '127.0.0.1', () => { - const { port } = probe.address() as AddressInfo; - probe.close(() => resolve(port)); - }); - }); -} - -async function startServer(): Promise { - const port = await freePort(); - server = spawn(process.execPath, [entry, 'http', '-p', String(port)], { - cwd: root, - env: serverEnvironment({ scrub: ['MCP_HTTP_BIND'], exitOnStdinClose: false }), - stdio: ['ignore', 'pipe', 'pipe'], - windowsHide: true, - }); - server.stdout!.on('data', (chunk: Buffer) => { serverOutput += chunk.toString(); }); - server.stderr!.on('data', (chunk: Buffer) => { serverOutput += chunk.toString(); }); - expect(await waitForHealthEndpoint(port, 20_000), serverOutput).toBe(true); - return port; -} - -async function connect(port: number, name: string): Promise { - const client = new Client({ name, version: '1.0.0' }); - await client.connect(new StreamableHTTPClientTransport(new URL(`http://127.0.0.1:${port}/mcp`))); - clients.push(client); - return client; -} - -async function call(client: Client, name: string, args: Record = {}): Promise { - const raw = await client.callTool({ name, arguments: args }, { timeout: 30_000 }); - const text = (raw.content as Array<{ type: string; text?: string }>).find(item => item.type === 'text')?.text; - expect(text, JSON.stringify(raw)).toBeDefined(); - return JSON.parse(text!) as Answer; -} - -afterEach(async () => { - for (const client of clients.splice(0)) { - await client.close().catch(() => {}); - } - if (server && server.exitCode === null) { - const exited = new Promise((resolve) => server!.once('exit', () => resolve())); - server.kill(); - await Promise.race([exited, new Promise((resolve) => setTimeout(resolve, 5_000))]); - } - server = undefined; - serverOutput = ''; -}); - -describe('Streamable HTTP: a debug session named by another client (issue #939)', () => { - it('answers whose it is instead of "not found", and the owner keeps it', async () => { - const port = await startServer(); - const owner = await connect(port, 'owner'); - const other = await connect(port, 'other'); - - const created = await call(owner, 'create_debug_session', { language: 'mock', name: 'owned' }); - expect(created.success, JSON.stringify(created)).toBe(true); - const sessionId = created.sessionId!; - - // The odd shapes too: close_debug_session answers with `message`, get_output - // with its own literal — the gate sits in front of every tool. - const closed = await call(other, 'close_debug_session', { sessionId }); - expect(closed.success).toBe(false); - expect(closed.error).toMatch(/belongs to another MCP client session/); - expect(closed.error).toMatch(/each Streamable HTTP client sees only the debug sessions it created/); - const output = await call(other, 'get_output', { sessionId }); - expect(output.success).toBe(false); - expect(output.error).toMatch(/belongs to another MCP client session/); - - // Still the owner's, still invisible to the other client. - expect((await call(other, 'list_debug_sessions')).sessions).toEqual([]); - expect((await call(owner, 'list_debug_sessions')).sessions?.map(session => session.id)).toContain(sessionId); - expect((await call(owner, 'close_debug_session', { sessionId })).success).toBe(true); - - // Gone everywhere: the plain answer, unchanged. - const gone = await call(other, 'close_debug_session', { sessionId }); - expect(gone.success).toBe(false); - expect(gone.message).toMatch(/Failed to close debug session/); - }, 60_000); -}); diff --git a/tests/integration/cli/http-host-allowlist.test.ts b/tests/integration/cli/http-host-allowlist.test.ts index 8587e418..1a122108 100644 --- a/tests/integration/cli/http-host-allowlist.test.ts +++ b/tests/integration/cli/http-host-allowlist.test.ts @@ -7,6 +7,7 @@ import { describe, it, expect, afterEach, vi } from 'vitest'; import * as http from 'http'; import type { AddressInfo } from 'net'; import { createHttpApp } from '../../../src/cli/http-command.js'; +import type { DebugMcpServer } from '../../../src/server.js'; import { FakeCurrentProcess } from '../../test-utils/mocks/fake-current-process.js'; import type { Logger as WinstonLogger } from 'winston'; import { createMockLogger } from '../../test-utils/mocks/mock-logger.js'; @@ -29,9 +30,18 @@ async function listen(options: { allowedHost?: string[] } = {}, env: Record [], getSession: () => undefined, idleMsOf: () => undefined } + }; const app = createHttpApp( { port: '0', ...options }, - { logger: createMockLogger() as unknown as WinstonLogger, serverFactory: vi.fn(), proc } + { logger: createMockLogger() as unknown as WinstonLogger, serverFactory: vi.fn(() => core as unknown as DebugMcpServer), proc } ); const server = app.listen(0, '127.0.0.1'); servers.push(server); diff --git a/tests/integration/cli/http-shared-core.test.ts b/tests/integration/cli/http-shared-core.test.ts new file mode 100644 index 00000000..dd950eb9 --- /dev/null +++ b/tests/integration/cli/http-shared-core.test.ts @@ -0,0 +1,162 @@ +/** + * Streamable HTTP: one debugger core per process (issue #1025, step 2). A + * debug session is addressable by whoever holds its id — the handle is the + * capability — so two clients of one server see the same list, either can + * close what the other created, and a debug session outlives the MCP session + * that created it. The per-client server and its "belongs to another MCP + * client session" refusal (#939) were the 2025 era's behaviour; the + * 2026-07-28 revision has no client sessions to key them on. + * + * This suite also measures the premise the idle policy's "attended" rule rests + * on: a v2 SDK client in legacy mode holds a standalone SSE stream open for the + * life of its connection, so /health reports the process attended while it is + * connected and unattended once it has closed. Real server, two SDK clients, + * the mock adapter — no toolchain. + */ +import { afterEach, describe, expect, it } from 'vitest'; +import { spawn, type ChildProcess } from 'node:child_process'; +import net, { type AddressInfo } from 'node:net'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { Client, StreamableHTTPClientTransport } from '@modelcontextprotocol/client'; +import { serverEnvironment } from '../../test-utils/helpers/mcp-stdio-client.js'; +import { waitForHealthEndpoint } from '../../e2e/smoke-test-utils.js'; + +const root = fileURLToPath(new URL('../../../', import.meta.url)); +const entry = path.join(root, 'dist/index.js'); + +interface Answer { + success: boolean; + error?: string; + message?: string; + sessionId?: string; + sessions?: Array<{ id: string }>; +} + +interface Health { + attended: boolean; + listenStreams: number; + sessions: string[]; + debugSessions: Array<{ id: string; idleMs?: number }>; +} + +let server: ChildProcess | undefined; +let serverOutput = ''; +const clients: Client[] = []; + +function freePort(): Promise { + return new Promise((resolve, reject) => { + const probe = net.createServer(); + probe.on('error', reject); + probe.listen(0, '127.0.0.1', () => { + const { port } = probe.address() as AddressInfo; + probe.close(() => resolve(port)); + }); + }); +} + +async function startServer(): Promise { + const port = await freePort(); + server = spawn(process.execPath, [entry, 'http', '-p', String(port)], { + cwd: root, + env: serverEnvironment({ scrub: ['MCP_HTTP_BIND'], exitOnStdinClose: false }), + stdio: ['ignore', 'pipe', 'pipe'], + windowsHide: true, + }); + server.stdout!.on('data', (chunk: Buffer) => { serverOutput += chunk.toString(); }); + server.stderr!.on('data', (chunk: Buffer) => { serverOutput += chunk.toString(); }); + expect(await waitForHealthEndpoint(port, 20_000), serverOutput).toBe(true); + return port; +} + +async function connect(port: number, name: string): Promise<{ client: Client; transport: StreamableHTTPClientTransport }> { + const client = new Client({ name, version: '1.0.0' }); + const transport = new StreamableHTTPClientTransport(new URL(`http://127.0.0.1:${port}/mcp`)); + await client.connect(transport); + clients.push(client); + return { client, transport }; +} + +async function call(client: Client, name: string, args: Record = {}): Promise { + const raw = await client.callTool({ name, arguments: args }, { timeout: 30_000 }); + const text = (raw.content as Array<{ type: string; text?: string }>).find(item => item.type === 'text')?.text; + expect(text, JSON.stringify(raw)).toBeDefined(); + return JSON.parse(text!) as Answer; +} + +async function health(port: number): Promise { + const response = await fetch(`http://127.0.0.1:${port}/health`); + return (await response.json()) as Health; +} + +/** Polls /health until `predicate` holds or the deadline passes; answers the last reading. */ +async function healthUntil(port: number, predicate: (h: Health) => boolean, deadlineMs = 10_000): Promise { + const deadline = Date.now() + deadlineMs; + let last = await health(port); + while (!predicate(last) && Date.now() < deadline) { + await new Promise((resolve) => setTimeout(resolve, 100)); + last = await health(port); + } + return last; +} + +afterEach(async () => { + for (const client of clients.splice(0)) { + await client.close().catch(() => {}); + } + if (server && server.exitCode === null) { + const exited = new Promise((resolve) => server!.once('exit', () => resolve())); + server.kill(); + await Promise.race([exited, new Promise((resolve) => setTimeout(resolve, 5_000))]); + } + server = undefined; + serverOutput = ''; +}); + +describe('Streamable HTTP: one core per process (issue #1025, step 2)', () => { + it('two clients see one list, either can drive the other\'s session, and a session outlives its MCP session', async () => { + const port = await startServer(); + const a = await connect(port, 'agent-a'); + const b = await connect(port, 'agent-b'); + + const created = await call(a.client, 'create_debug_session', { language: 'mock', name: 'shared' }); + expect(created.success, JSON.stringify(created)).toBe(true); + const sessionId = created.sessionId!; + + // The whole process's list, from either client; the id is the capability. + expect((await call(b.client, 'list_debug_sessions')).sessions?.map(session => session.id)).toContain(sessionId); + const output = await call(b.client, 'get_output', { sessionId }); + expect(output.success, JSON.stringify(output)).toBe(true); + expect((await health(port)).debugSessions.map(session => session.id)).toContain(sessionId); + + // Client A ends its MCP session cleanly (DELETE /mcp). The connection is gone; + // the debug session is not: it answers to the idle policy, not to the MCP session. + await a.transport.terminateSession(); + await a.client.close().catch(() => {}); + const afterDelete = await healthUntil(port, (h) => h.sessions.length === 1); + expect(afterDelete.sessions).toHaveLength(1); + expect(afterDelete.debugSessions.map(session => session.id)).toContain(sessionId); + expect((await call(b.client, 'list_debug_sessions')).sessions?.map(session => session.id)).toContain(sessionId); + + // Client B closes what A created. + expect((await call(b.client, 'close_debug_session', { sessionId })).success).toBe(true); + const gone = await call(b.client, 'close_debug_session', { sessionId }); + expect(gone.success).toBe(false); + expect(gone.message).toMatch(/Failed to close debug session/); + }, 60_000); + + it('a connected legacy client attends the process for the life of its connection (the idle policy\'s premise)', async () => { + const port = await startServer(); + expect((await health(port)).attended).toBe(false); + + const a = await connect(port, 'agent-a'); + // No tool call at all: presence alone, through the SDK client's standalone SSE stream. + const connected = await healthUntil(port, (h) => h.attended); + expect(connected.attended, JSON.stringify(connected)).toBe(true); + expect(connected.listenStreams).toBe(0); + + await a.client.close(); + const closed = await healthUntil(port, (h) => !h.attended); + expect(closed.attended, JSON.stringify(closed)).toBe(false); + }, 60_000); +}); diff --git a/tests/unit/cli/http-command.test.ts b/tests/unit/cli/http-command.test.ts index 8a2d5bc8..2f04d7fe 100644 --- a/tests/unit/cli/http-command.test.ts +++ b/tests/unit/cli/http-command.test.ts @@ -47,6 +47,8 @@ describe('HTTP Command Handler', () => { // Track transports created so tests can drive them let createdTransports: any[]; let lastTransportOptions: any; + // Whether the transport mock completes an initialize handshake inside handleRequest (the SDK does). + let autoInitialize = true; beforeEach(() => { mockLogger = { @@ -62,9 +64,12 @@ describe('HTTP Command Handler', () => { mockServer = { start: vi.fn().mockResolvedValue(undefined), stop: vi.fn().mockResolvedValue(undefined), + setAttendedSupplier: vi.fn(), + idlePolicy: { idleMs: 1_800_000, sweepMs: 60_000 }, sessionManager: { getAllSessions: vi.fn().mockReturnValue([]), getSession: vi.fn().mockReturnValue(undefined), + idleMsOf: vi.fn().mockReturnValue(undefined), }, } as any; mockFacade = { connect: vi.fn<() => Promise>().mockResolvedValue(undefined) }; @@ -77,18 +82,23 @@ describe('HTTP Command Handler', () => { fakeProc = new FakeCurrentProcess(); createdTransports = []; + autoInitialize = true; MockedStreamableHTTPServerTransport.mockImplementation(function (options: any) { lastTransportOptions = options; const sessionId = 'session-' + Math.random().toString(36).slice(2, 9); const t: any = { - sessionId, + // As in the SDK, the id exists only once initialize has completed. + sessionId: undefined, close: vi.fn(), onclose: undefined, onerror: undefined, - handleRequest: vi.fn().mockResolvedValue(undefined), - // Helper: drive the SDK's onsessioninitialized callback to register the session + handleRequest: vi.fn(), + // Helper: drive the SDK's onsessioninitialized callback to register the + // session. Idempotent, like the SDK's own one-shot callback. triggerSessionInit() { + if (this.sessionId) return; + this.sessionId = sessionId; if (options?.onsessioninitialized) options.onsessioninitialized(sessionId); }, triggerClose() { @@ -98,6 +108,14 @@ describe('HTTP Command Handler', () => { if (this.onerror) this.onerror(err); }, }; + // The SDK completes the handshake inside handleRequest: a successful + // initialize registers the session before the call resolves. A suite + // that wants a refused handshake sets autoInitialize = false first. + t.handleRequest.mockImplementation(async (_req: unknown, _res: unknown, body: any) => { + if (autoInitialize && body?.method === 'initialize') { + t.triggerSessionInit(); + } + }); createdTransports.push(t); mockTransport = t; return t; @@ -404,7 +422,7 @@ describe('HTTP Command Handler', () => { } }); - it('removes the session from the map and stops its server when the transport closes', async () => { + it('removes the session from the map when the transport closes and leaves the one core running (issue #1025)', async () => { const { app, handler } = getHandler(); await handler( makeReq({ @@ -416,11 +434,54 @@ describe('HTTP Command Handler', () => { expect((app as any).httpSessions.size).toBe(1); mockTransport.triggerClose(); - // Allow async stop() to settle await new Promise((r) => setImmediate(r)); expect((app as any).httpSessions.size).toBe(0); - expect(mockServer.stop).toHaveBeenCalled(); + // The debug sessions live on under the idle policy; the protocol session was only a connection. + expect(mockServer.stop).not.toHaveBeenCalled(); + }); + + it('closes a transport whose initialize never completed, so nothing leaks (issue #1025)', async () => { + const { app, handler } = getHandler(); + autoInitialize = false; + await handler( + makeReq({ + body: { jsonrpc: '2.0', id: 1, method: 'initialize', params: { protocolVersion: '2025-11-25', capabilities: {}, clientInfo: { name: 'c', version: '1' } } }, + }), + makeRes() + ); + // onsessioninitialized never fired: the SDK refused the handshake. + + expect(mockTransport.close).toHaveBeenCalledTimes(1); + expect((app as any).httpSessions.size).toBe(0); + }); + + it('builds one core for the process and tells it who is attending (issue #1025)', async () => { + const { app, handler } = getHandler(); + expect(mockServerFactory).toHaveBeenCalledTimes(1); + expect(mockServerFactory).toHaveBeenCalledWith({ + logLevel: undefined, + logFile: undefined, + debugSessionIdle: { defaultMs: 1_800_000 }, + }); + expect(mockServer.setAttendedSupplier).toHaveBeenCalledTimes(1); + const attended = vi.mocked(mockServer.setAttendedSupplier).mock.calls[0][0] as () => boolean; + expect(attended()).toBe(false); + + await handler( + makeReq({ + body: { jsonrpc: '2.0', id: 1, method: 'initialize', params: { protocolVersion: '2025-11-25', capabilities: {}, clientInfo: { name: 'c', version: '1' } } }, + }), + makeRes() + ); + mockTransport.triggerSessionInit(); + // A second MCP session is a second facade over the same core, not a second core. + expect(mockServerFactory).toHaveBeenCalledTimes(1); + expect(mockProtocolServerFactory).toHaveBeenCalledTimes(1); + expect(attended()).toBe(false); + + (app as any).httpSessions.get(mockTransport.sessionId).openStreams = 1; + expect(attended()).toBe(true); }); it('logs and surfaces transport errors', async () => { @@ -515,14 +576,16 @@ describe('HTTP Command Handler', () => { mode: 'http', listening: { address: '127.0.0.1', port: 3001 }, connections: 0, + attended: false, + listenStreams: 0, sessions: [], details: [], + debugSessions: [], }); // Add a session and re-check (app as any).httpSessions.set('s1', { transport: {}, - server: mockServer, lastActivity: Date.now(), openStreams: 1, hadStream: true, @@ -532,11 +595,12 @@ describe('HTTP Command Handler', () => { status: 'ok', mode: 'http', connections: 1, + attended: true, sessions: ['s1'], })); }); - it('names what each HTTP session holds, so an orphan is discoverable (issue #658)', () => { + it('names every MCP session and every debug session of the process, so an orphan is discoverable (issues #658, #1025)', () => { vi.useFakeTimers(); vi.setSystemTime(100_000); const app = createHttpApp( @@ -546,10 +610,11 @@ describe('HTTP Command Handler', () => { const healthHandler = mockApp.get.mock.calls.find((c: any) => c[0] === '/health')![1]; (mockServer as any).sessionManager.getAllSessions.mockReturnValue([ { id: 'dbg-1', name: 'attached', language: 'javascript', state: 'paused', createdAt: new Date(), extra: 'omitted' }, + { id: 'dbg-2', name: 'worker', language: 'javascript', state: 'running', createdAt: new Date(), parentSessionId: 'dbg-1' }, ]); + (mockServer as any).sessionManager.idleMsOf.mockImplementation((id: string) => (id === 'dbg-1' || id === 'dbg-2' ? 75_000 : undefined)); (app as any).httpSessions.set('orphan', { transport: {}, - server: mockServer, lastActivity: 40_000, openStreams: 0, hadStream: true, @@ -557,15 +622,13 @@ describe('HTTP Command Handler', () => { const res = { json: vi.fn() }; healthHandler({}, res); - expect(res.json.mock.calls[0][0].details).toEqual([ - { - id: 'orphan', - openStreams: 0, - streamLost: true, - idleMs: 60_000, - debugSessions: [{ id: 'dbg-1', name: 'attached', language: 'javascript', state: 'paused' }], - }, + const body = res.json.mock.calls[0][0]; + expect(body.details).toEqual([{ id: 'orphan', openStreams: 0, streamLost: true, idleMs: 60_000 }]); + expect(body.debugSessions).toEqual([ + { id: 'dbg-1', name: 'attached', language: 'javascript', state: 'paused', idleMs: 75_000 }, + { id: 'dbg-2', name: 'worker', language: 'javascript', state: 'running', parentSessionId: 'dbg-1', idleMs: 75_000 }, ]); + expect(body.attended).toBe(false); }); }); @@ -619,9 +682,10 @@ describe('HTTP Command Handler', () => { expect(transport.close).toHaveBeenCalled(); // The SDK fires onclose from close(); simulate it and confirm the - // existing cleanup path runs: server stopped, session forgotten. + // session is forgotten. The core stays: a reaped MCP session is a dead + // connection, and its debug sessions answer to the idle policy (#1025). transport.triggerClose(); - expect(mockServer.stop).toHaveBeenCalled(); + expect(mockServer.stop).not.toHaveBeenCalled(); expect((app as any).httpSessions.size).toBe(0); }); @@ -726,8 +790,10 @@ describe('HTTP Command Handler', () => { await vi.advanceTimersByTimeAsync(60_000); expect(transport.close).toHaveBeenCalled(); expect(mockLogger.warn).toHaveBeenCalledWith( - expect.stringMatching(/its SSE stream closed 180s ago .*MCP_HTTP_STREAM_LOST_SESSION_MS=120000.*holding debug session\(s\) dbg-1 \(javascript, paused\)/) + expect.stringMatching(/its SSE stream closed 180s ago .*MCP_HTTP_STREAM_LOST_SESSION_MS=120000/) ); + // The MCP session held nothing of its own any more (#1025): no "holding" suffix. + expect(mockLogger.warn).not.toHaveBeenCalledWith(expect.stringMatching(/holding debug session/)); }); it('a stream that comes back within the window keeps the session alive (issue #658)', async () => { @@ -1143,7 +1209,33 @@ describe('HTTP Command Handler', () => { expect(fakeProc.stderrChunks.join('')).toMatch(/3001.*already in use/); }); - it('handles SIGINT by closing all transports, stopping all servers, then exiting', async () => { + it('starts the one core before listening, and never again per MCP session (issue #1025)', async () => { + const listen = vi.fn(() => mockHttpServer); + mockApp.listen = listen; + + await handleHttpCommand( + { port: '3001' }, + { logger: mockLogger, serverFactory: mockServerFactory, protocolServerFactory: mockProtocolServerFactory, exitProcess: mockExitProcess, proc: fakeProc } + ); + + expect(mockServerFactory).toHaveBeenCalledTimes(1); + expect(mockServer.start).toHaveBeenCalledTimes(1); + expect(vi.mocked(mockServer.start).mock.invocationCallOrder[0]).toBeLessThan(listen.mock.invocationCallOrder[0]); + + const postCall = mockApp.post.mock.calls.find((c: any) => c[0] === '/mcp'); + const handler = postCall![1] as (req: any, res: any) => Promise; + const initBody = { jsonrpc: '2.0', id: 1, method: 'initialize', params: { protocolVersion: '2025-11-25', capabilities: {}, clientInfo: { name: 'c', version: '1' } } }; + await handler({ method: 'POST', headers: {}, body: initBody }, { status: vi.fn().mockReturnThis(), json: vi.fn(), end: vi.fn(), headersSent: false }); + mockTransport.triggerSessionInit(); + await handler({ method: 'POST', headers: {}, body: initBody }, { status: vi.fn().mockReturnThis(), json: vi.fn(), end: vi.fn(), headersSent: false }); + mockTransport.triggerSessionInit(); + + expect(mockServerFactory).toHaveBeenCalledTimes(1); + expect(mockServer.start).toHaveBeenCalledTimes(1); + expect(mockProtocolServerFactory).toHaveBeenCalledTimes(2); + }); + + it('handles SIGINT by closing all transports, stopping the one core, then exiting', async () => { const listen = vi.fn(() => mockHttpServer); mockApp.listen = listen; @@ -1158,18 +1250,17 @@ describe('HTTP Command Handler', () => { // Inject mock sessions const t1 = { close: vi.fn() }; const t2 = { close: vi.fn() }; - const s1 = { stop: vi.fn().mockResolvedValue(undefined) }; - const s2 = { stop: vi.fn().mockResolvedValue(undefined) }; const sessions = (mockApp as any).httpSessions as Map; - sessions.set('a', { transport: t1, server: s1 }); - sessions.set('b', { transport: t2, server: s2 }); + sessions.set('a', { transport: t1 }); + sessions.set('b', { transport: t2 }); await sigintHandler(); expect(t1.close).toHaveBeenCalled(); expect(t2.close).toHaveBeenCalled(); - expect(s1.stop).toHaveBeenCalled(); - expect(s2.stop).toHaveBeenCalled(); + expect(mockServer.stop).toHaveBeenCalledTimes(1); + // Transports first, so no request lands on a core that is closing its sessions. + expect(vi.mocked(mockServer.stop).mock.invocationCallOrder[0]).toBeGreaterThan(t2.close.mock.invocationCallOrder[0]); expect(mockHttpServer.close).toHaveBeenCalled(); expect(mockExitProcess).toHaveBeenCalledWith(0); }); @@ -1185,11 +1276,9 @@ describe('HTTP Command Handler', () => { const sigintHandler = fakeProc.lastListener('SIGINT'); const sessions = (mockApp as any).httpSessions as Map; - // A wedged proxy makes stop() hang — shutdown must not hang with it. - sessions.set('wedged', { - transport: { close: vi.fn() }, - server: { stop: vi.fn().mockReturnValue(new Promise(() => {})) } - }); + // A wedged proxy makes the core's stop() hang — shutdown must not hang with it. + sessions.set('wedged', { transport: { close: vi.fn() } }); + mockServer.stop = vi.fn().mockReturnValue(new Promise(() => {})); vi.useFakeTimers(); const shutdownPromise = sigintHandler(); @@ -1250,14 +1339,13 @@ describe('HTTP Command Handler', () => { // Inject an active session to prove graceful shutdown ran const t1 = { close: vi.fn() }; - const s1 = { stop: vi.fn().mockResolvedValue(undefined) }; - ((mockApp as any).httpSessions as Map).set('a', { transport: t1, server: s1 }); + ((mockApp as any).httpSessions as Map).set('a', { transport: t1 }); stdin.emit('end'); await vi.waitFor(() => expect(mockExitProcess).toHaveBeenCalledWith(0)); expect(t1.close).toHaveBeenCalled(); - expect(s1.stop).toHaveBeenCalled(); + expect(mockServer.stop).toHaveBeenCalled(); expect(mockHttpServer.close).toHaveBeenCalled(); }); diff --git a/tests/unit/cli/stdio-command.test.ts b/tests/unit/cli/stdio-command.test.ts index d9c789ca..5fbe785b 100644 --- a/tests/unit/cli/stdio-command.test.ts +++ b/tests/unit/cli/stdio-command.test.ts @@ -85,10 +85,12 @@ describe('STDIO Command Handler', () => { // Verify info log was called expect(mockLogger.info).toHaveBeenCalledWith('Starting Debug MCP Server in stdio mode'); - // Verify server factory was called with correct options + // Verify server factory was called with correct options: on stdio the pipe + // is the lifetime, so the idle policy defaults to off (issue #1025, step 2) expect(mockServerFactory).toHaveBeenCalledWith({ logLevel: 'debug', - logFile: '/tmp/test.log' + logFile: '/tmp/test.log', + debugSessionIdle: { defaultMs: 0 } }); // Verify server start was called @@ -151,7 +153,8 @@ describe('STDIO Command Handler', () => { // Verify server factory was called with correct options expect(mockServerFactory).toHaveBeenCalledWith({ logLevel: undefined, - logFile: undefined + logFile: undefined, + debugSessionIdle: { defaultMs: 0 } }); }); From d7d706f611f85148121867818898047c809d74b4 Mon Sep 17 00:00:00 2001 From: JF Date: Mon, 9 Nov 2026 20:50:55 -0500 Subject: [PATCH 4/8] fix(http): count a finished wait as activity, close a transport whose handshake threw, and state the isolation seam's limits (#1025) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups on the shared core: - A completed wait_for_stop refreshes the tree's idle clock when it settles, so an agent that returns a few minutes after a long wait does not find its session swept. - A child adoption refreshes the root's clock (the tree is alive). - A stray sessionId on a tool whose schema takes none refreshes nothing, so a caller cannot keep a tree it may not see alive by naming it. - The idle sweep asks the attended supplier again before each close: a client arriving while an earlier close runs stops the rest. The supplier itself travels into the sweep. - A new MCP session's transport is closed in `finally` when its handshake did not complete, whether the SDK refused it or the transport threw, so no facade sink is left in the process-wide hub. - One attendance predicate feeds both the supplier and /health. - The attendance measurement holds: /health is read again 2.5 s after the first attended reading and must still show the stream open. - The unit test for a refused handshake asserts the clean path (no 500, no error log); the transport mock's close resolves like the SDK's. - Docs: no authenticated http mode ships yet, and an authenticating reverse proxy does not identify callers to the server — SECURITY.md, the tool reference and the ownership fragment say so instead of implying per-principal scoping is available today; the jit-diagnostics /health sentence and the fragment name the new shape. Co-Authored-By: Claude Fable 5.1 --- SECURITY.md | 2 +- changelog.d/1025-ownership.changed.md | 2 +- docs/jit-diagnostics/README.md | 2 +- docs/tool-reference.md | 4 +-- src/cli/http-command.ts | 36 ++++++++++--------- src/server.ts | 3 +- src/server/handlers/execution-tools.ts | 2 ++ src/server/tool-dispatch.ts | 15 ++++---- src/session/session-manager-core.ts | 13 +++++-- .../server/handlers/execution-tools.test.ts | 3 ++ .../core/unit/server/server-lifecycle.test.ts | 15 ++++++-- .../server/tool-dispatch-owner-key.test.ts | 11 ++++++ .../unit/session/session-idle-policy.test.ts | 26 ++++++++++++++ .../integration/cli/http-shared-core.test.ts | 11 +++++- tests/unit/cli/http-command.test.ts | 33 +++++++++++++++-- 15 files changed, 141 insertions(+), 37 deletions(-) diff --git a/SECURITY.md b/SECURITY.md index 4ee4d571..59093078 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -68,7 +68,7 @@ If you want to constrain what the MCP client can reach, constrain the mcp-debugg - **Containers.** Run the published `debugmcp/mcp-debugger` image with the workspace mounted at `/workspace`. The container boundary is the filesystem boundary. - **OS permissions.** Run mcp-debugger as a user with read access only to the source you want it to debug, and no access to secrets, SSH keys, credentials, etc. The kernel enforces the limits. -- **Network exposure.** If running in http mode, treat the port as a remote shell endpoint — anyone who can reach it inherits the mcp-debugger process's privileges. Bind to loopback or put it behind authenticated network access. One http process is single-tenant: every client of it sees and can drive every debug session (a session id is the capability); run one process per agent where that matters. With authentication in front of `/mcp`, debug sessions are scoped to the authenticated principal that created them. +- **Network exposure.** If running in http mode, treat the port as a remote shell endpoint — anyone who can reach it inherits the mcp-debugger process's privileges. Bind to loopback or put it behind authenticated network access. One http process is single-tenant: every client of it sees and can drive every debug session (a session id is the capability); run one process per agent where that matters. The server is built to scope sessions per principal once it verifies callers itself, but no authenticated http mode ships yet, and an authenticating reverse proxy in front of `/mcp` does not identify callers to the server — every caller still shares one scope. - **Remote debuggees.** When using `attach_to_process` to debug a process on another host, the MCP client inherits that debuggee's privileges too. Apply the same containment reasoning there. - **MCP client trust.** Do not expose mcp-debugger to an MCP client that you trust less than the OS user mcp-debugger runs under. - **DAP mirror endpoints.** `expose_session` opens a per-session DAP endpoint bound to `127.0.0.1` and gated by a random per-expose token. The mirror rejects execution control and breakpoint changes, but it forwards `evaluate` — and DAP evaluate can execute arbitrary code in the debuggee — so **treat the mirror token as a debuggee-execution capability, not a view-only credential**. Any local process that obtains the token gets that capability; the token is returned only in the `expose_session` result and is redacted from logs. "Read-only" means control is rejected, not that the debuggee is immutable. diff --git a/changelog.d/1025-ownership.changed.md b/changelog.d/1025-ownership.changed.md index dba7a5e1..ac016155 100644 --- a/changelog.d/1025-ownership.changed.md +++ b/changelog.d/1025-ownership.changed.md @@ -1 +1 @@ -**One debugger core per `http` process: every MCP client sees the same debug sessions.** A Streamable HTTP server used to build an isolated server per MCP client session, so `list_debug_sessions` listed only the caller's own sessions and another client's id was refused with "belongs to another MCP client session" (#939). The 2026-07-28 revision of the protocol has no client sessions to key that on, so the model is now the one the spec names — the handle is the capability: a debug session is addressable by whoever holds its id, `list_debug_sessions` is the whole process's list, and several agents against one `http` process see each other's sessions (one process per agent is the isolation, as `stdio` already gives). Debug sessions therefore outlive the MCP session that created them, and a client that reconnects can list and resume or close what it left behind. The seam for real isolation is in place: a session is created in its caller's scope — the authenticated principal when the request carried one, otherwise the shared anonymous scope — and an out-of-scope id answers as unknown; with no authentication configured every caller is anonymous, which is today's single-tenant behaviour. `GET /health` now lists the process's debug sessions beside the MCP sessions, and says whether a client is attending. The ten-per-language adapter cap is per process now, as it always was on `stdio`. Step 2 of #1025. +**One debugger core per `http` process: every MCP client sees the same debug sessions.** A Streamable HTTP server used to build an isolated server per MCP client session, so `list_debug_sessions` listed only the caller's own sessions and another client's id was refused with "belongs to another MCP client session" (#939). The 2026-07-28 revision of the protocol has no client sessions to key that on, so the model is now the one the spec names — the handle is the capability: a debug session is addressable by whoever holds its id, `list_debug_sessions` is the whole process's list, and several agents against one `http` process see each other's sessions (one process per agent is the isolation, as `stdio` already gives). Debug sessions therefore outlive the MCP session that created them, and a client that reconnects can list and resume or close what it left behind. The seam for real isolation is in place: a session is created in its caller's scope — the authenticated principal when the request carried one, otherwise the shared anonymous scope — and an out-of-scope id answers as unknown; no authenticated `http` mode ships yet (an authenticating reverse proxy does not identify callers to the server), so every caller is anonymous and sees everything, which is today's single-tenant behaviour. `GET /health`'s `details[]` no longer carries a per-client `debugSessions` list; the process-wide `debugSessions` array replaces it. `GET /health` now lists the process's debug sessions beside the MCP sessions, and says whether a client is attending. The ten-per-language adapter cap is per process now, as it always was on `stdio`. Step 2 of #1025. diff --git a/docs/jit-diagnostics/README.md b/docs/jit-diagnostics/README.md index 2caffab9..59d392bd 100644 --- a/docs/jit-diagnostics/README.md +++ b/docs/jit-diagnostics/README.md @@ -128,7 +128,7 @@ kubectl port-forward "pod/$POD" 3001:3001 & Register the endpoint with your agent (e.g. `claude mcp add-json sick-pod '{"type":"http","url":"http://127.0.0.1:3001/mcp"}'`). -The HTTP server closes debug sessions abandoned by a crashed client, releasing any ptrace claim on the target, through the per-debug-session idle policy: a session tree no tool call has named for 30 minutes is closed unless a client is attending the process (`MCP_DEBUG_SESSION_IDLE_MS`; `0` disables). For a short-lived diagnostic sidecar a tighter window is safer — add `--env=MCP_DEBUG_SESSION_IDLE_MS=300000` (5 minutes) to the `kubectl debug` command. The abandoned MCP sessions themselves are reaped separately: a client whose SSE stream dropped and never returned (any MCP SDK client that crashed or exited without `DELETE`) is reaped after 2 minutes idle (`MCP_HTTP_STREAM_LOST_SESSION_MS`), one that never opened a stream after 30 (`MCP_HTTP_STALE_SESSION_MS`). For a short-lived diagnostic sidecar a tighter stale window is safer — add `--env=MCP_HTTP_STALE_SESSION_MS=300000` (5 minutes; `0` disables) to the `kubectl debug` command. `GET /health` on the sidecar lists what each MCP session is holding. +The HTTP server closes debug sessions abandoned by a crashed client, releasing any ptrace claim on the target, through the per-debug-session idle policy: a session tree no tool call has named for 30 minutes is closed unless a client is attending the process (`MCP_DEBUG_SESSION_IDLE_MS`; `0` disables). For a short-lived diagnostic sidecar a tighter window is safer — add `--env=MCP_DEBUG_SESSION_IDLE_MS=300000` (5 minutes) to the `kubectl debug` command. The abandoned MCP sessions themselves are reaped separately: a client whose SSE stream dropped and never returned (any MCP SDK client that crashed or exited without `DELETE`) is reaped after 2 minutes idle (`MCP_HTTP_STREAM_LOST_SESSION_MS`), one that never opened a stream after 30 (`MCP_HTTP_STALE_SESSION_MS`). For a short-lived diagnostic sidecar a tighter stale window is safer — add `--env=MCP_HTTP_STALE_SESSION_MS=300000` (5 minutes; `0` disables) to the `kubectl debug` command. `GET /health` on the sidecar lists the process's debug sessions with their idle time, and whether a client is attending. Facts that will save you an afternoon (all observed, not assumed): diff --git a/docs/tool-reference.md b/docs/tool-reference.md index f1280de4..c82d78f0 100644 --- a/docs/tool-reference.md +++ b/docs/tool-reference.md @@ -87,7 +87,7 @@ Creates a new debugging session. **Notes:** - Session IDs are UUIDs in the format `xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx` - Sessions start in `"created"` state -- **One debugger core per process; the id is the capability (issue #1025).** On Streamable HTTP every MCP client of a process sees the same debug sessions: `list_debug_sessions` is the whole process's list, and any client holding an id can drive or close that session — run one `http` process per agent where agents must not see each other's work. A debug session outlives the MCP session that created it, so a one-shot script that opens a fresh MCP session per call reaches the sessions its previous run created, and a client that reconnects can resume or close what it left behind. What ends an abandoned session is the **idle policy**: a session tree that no tool call has named for `MCP_DEBUG_SESSION_IDLE_MS` (default 30 minutes on `http`, off on `stdio`; `0` disables) is closed through the normal close cascade, unless a client is attending the process — while any client holds its SSE stream open, nothing expires; an in-flight `wait_for_stop` holds its tree; children never expire on a clock of their own. Explicit close and debuggee exit end a session sooner, as always. With authentication in front of `/mcp`, sessions are scoped to the principal that created them and an out-of-scope id answers as unknown; with none, every caller is in the one shared scope +- **One debugger core per process; the id is the capability (issue #1025).** On Streamable HTTP every MCP client of a process sees the same debug sessions: `list_debug_sessions` is the whole process's list, and any client holding an id can drive or close that session — run one `http` process per agent where agents must not see each other's work. A debug session outlives the MCP session that created it, so a one-shot script that opens a fresh MCP session per call reaches the sessions its previous run created, and a client that reconnects can resume or close what it left behind. What ends an abandoned session is the **idle policy**: a session tree that no tool call has named for `MCP_DEBUG_SESSION_IDLE_MS` (default 30 minutes on `http`, off on `stdio`; `0` disables) is closed through the normal close cascade, unless a client is attending the process — while any client holds its SSE stream open, nothing expires; an in-flight `wait_for_stop` holds its tree; children never expire on a clock of their own. Explicit close and debuggee exit end a session sooner, as always. The server is built to scope sessions per principal once it verifies callers itself (no authenticated `http` mode ships yet; an authenticating reverse proxy does not identify callers to the server), so today every caller is in the one shared scope - When `port`, `processId` or an `adapterConfig` URI/info-file key is provided in `create_debug_session`, the server performs an inline attach (creating the session and immediately attaching). The session exists before the addressing gate can refuse the form (a `port` on C/C++, say), so such a refusal answers `success: false` with a usable `sessionId`. The response then mirrors `attach_to_process`: alongside `sessionId` it carries `state`, the attach `data` payload, an optional `warning`, and — when a requested post-attach pause has not landed yet — `pending: true`, with `message` saying so (`…; post-attach pause pending — the target stops when it next executes code; call wait_for_stop to block until it does (pass stopOnEntry: false to attach without pausing)`) --- @@ -1821,7 +1821,7 @@ Session-lifecycle failures (unknown/terminated session, proxy not running) are a ``` ### Common Error Scenarios -1. **Session not found**: Occurs when a session terminates unexpectedly, or when the idle policy closed it (`MCP_DEBUG_SESSION_IDLE_MS`, see `create_debug_session`). An id created by another MCP client of the same process is found like any other; one outside the caller's scope (only with authentication in front of `http`) answers exactly as an unknown id (issue #1025) +1. **Session not found**: Occurs when a session terminates unexpectedly, or when the idle policy closed it (`MCP_DEBUG_SESSION_IDLE_MS`, see `create_debug_session`). An id created by another MCP client of the same process is found like any other; one outside the caller's scope (a future authenticated `http` mode; today every caller shares one scope) answers exactly as an unknown id (issue #1025) 2. **Invalid language**: Language must be one of the supported languages (discovered dynamically from installed adapters) 3. **File not found**: When setting breakpoints in non-existent files 4. **Invalid scope**: When passing wrong variablesReference to get_variables diff --git a/src/cli/http-command.ts b/src/cli/http-command.ts index df69d6e7..8e47eb35 100644 --- a/src/cli/http-command.ts +++ b/src/cli/http-command.ts @@ -212,15 +212,16 @@ export function createHttpApp( }); // While any client holds an open SSE stream the process is attended and the // idle policy closes nothing; the modern era's listen streams join this - // count when they are served. - core.setAttendedSupplier(() => { + // count when they are served. One predicate for the supplier and /health. + const attended = (): boolean => { for (const session of httpSessions.values()) { if (session.openStreams > 0) { return true; } } return false; - }); + }; + core.setAttendedSupplier(attended); // Reap crash-abandoned MCP sessions (issue #337): a client that dies without // DELETE leaves its transport registered forever — transport.onclose only @@ -298,14 +299,15 @@ export function createHttpApp( app.use(express.json({ limit: JSON_BODY_LIMIT })); const handleMcpRequest = async (req: Request, res: Response): Promise => { + // Set when this request opens a new MCP session, so a handshake the SDK + // refuses — or a transport fault during it — does not leave its transport + // and facade behind (issue #1025). + let opening: NodeStreamableHTTPServerTransport | undefined; try { const sessionIdHeader = req.headers['mcp-session-id']; const sessionId = Array.isArray(sessionIdHeader) ? sessionIdHeader[0] : sessionIdHeader; let transport: NodeStreamableHTTPServerTransport; - // Set when this request opens a new MCP session, so a handshake the SDK - // refuses does not leave its transport and facade behind (issue #1025). - let opening: NodeStreamableHTTPServerTransport | undefined; if (sessionId && httpSessions.has(sessionId)) { // Existing session — route to its transport @@ -391,15 +393,6 @@ export function createHttpApp( } await transport.handleRequest(req as IncomingMessage, res as ServerResponse, req.body); - if (opening && !opening.sessionId) { - // The handshake did not complete (the SDK refused it): nothing registered - // this transport, so nothing would ever close it. Closing it runs the - // facade's onclose chain and detaches its notification sink. - logger.debug('Closing the transport of an initialize that did not complete'); - await opening.close().catch((err: unknown) => { - logger.debug('Error closing an uninitialized transport', { error: err }); - }); - } } catch (error) { logger.error('Error handling MCP request', { error }); if (!res.headersSent) { @@ -407,6 +400,17 @@ export function createHttpApp( jsonRpcErrorBody(ProtocolErrorCode.InternalError, 'Internal error', error instanceof Error ? error.message : 'Unknown error') ); } + } finally { + if (opening && !opening.sessionId) { + // The handshake did not complete (the SDK refused it, or the transport + // threw): nothing registered this transport, so nothing would ever close + // it. Closing it runs the facade's onclose chain and detaches its + // notification sink from the process-wide hub. + logger.debug('Closing the transport of an initialize that did not complete'); + await opening.close().catch((err: unknown) => { + logger.debug('Error closing an uninitialized transport', { error: err }); + }); + } } }; @@ -430,7 +434,7 @@ export function createHttpApp( mode: 'http', listening: { ...listening }, connections: httpSessions.size, - attended: sessions.some((session) => session.openStreams > 0), + attended: attended(), listenStreams: 0, sessions: Array.from(httpSessions.keys()), details: sessions, diff --git a/src/server.ts b/src/server.ts index 636d2779..a9d338be 100644 --- a/src/server.ts +++ b/src/server.ts @@ -856,7 +856,8 @@ export class DebugMcpServer implements ToolContext { } this.idleSweepInFlight = true; try { - await this.sessionManager.sweepIdle(this.idlePolicy.idleMs, { attended: this.attendedSupplier() }); + // The supplier itself travels, so the sweep can ask again before each close. + await this.sessionManager.sweepIdle(this.idlePolicy.idleMs, { attended: this.attendedSupplier }); } catch (error) { this.logger.error('Idle sweep failed', { error }); } finally { diff --git a/src/server/handlers/execution-tools.ts b/src/server/handlers/execution-tools.ts index 0399da7d..2695959f 100644 --- a/src/server/handlers/execution-tools.ts +++ b/src/server/handlers/execution-tools.ts @@ -192,6 +192,8 @@ export const waitForStopTool: ToolHandler = async (ctx, args, _toolName, extra) throw error; } finally { releaseIdleHold(); + // The wait's end is activity too: the idle clock restarts when it settles. + ctx.sessionManager.touchActivity(args.sessionId); } }; diff --git a/src/server/tool-dispatch.ts b/src/server/tool-dispatch.ts index cdd7bf27..f1128253 100644 --- a/src/server/tool-dispatch.ts +++ b/src/server/tool-dispatch.ts @@ -26,11 +26,12 @@ import { TOOL_HANDLERS } from './handlers/index.js'; * own not-found answer. With no authentication configured every caller is in * the shared anonymous scope and sees everything, as before. */ +function takesSessionId(definition: ToolDefinition): boolean { + return Object.prototype.hasOwnProperty.call(definition.inputSchema.properties ?? {}, 'sessionId'); +} + function outOfScopeResult(ctx: ToolContext, definition: ToolDefinition, sessionId: unknown, callerKey: string | undefined): ToolResult | undefined { - if (typeof sessionId !== 'string') { - return undefined; - } - if (!Object.prototype.hasOwnProperty.call(definition.inputSchema.properties ?? {}, 'sessionId')) { + if (typeof sessionId !== 'string' || !takesSessionId(definition)) { return undefined; } if (!ctx.sessionManager.getSession(sessionId) || ctx.sessionManager.isSessionVisibleTo(sessionId, callerKey)) { @@ -103,8 +104,10 @@ export function registerToolHandlers(server: Server, ctx: ToolContext): void { assertRequiredToolArguments(definition, args as unknown as Record); const callerKey = ownerKeyOf(callCtx); const refused = outOfScopeResult(ctx, definition, args.sessionId, callerKey); - if (!refused && typeof args.sessionId === 'string') { - // A tool call naming a session is activity for the idle policy (issue #1025, step 2). + if (!refused && typeof args.sessionId === 'string' && takesSessionId(definition)) { + // A tool call naming a session is activity for the idle policy (issue + // #1025, step 2) — only through a tool that takes the id, so a stray + // argument cannot keep a tree alive the caller may not see. ctx.sessionManager.touchActivity(args.sessionId); } const result = diff --git a/src/session/session-manager-core.ts b/src/session/session-manager-core.ts index 7dafbf62..7d48ad5a 100644 --- a/src/session/session-manager-core.ts +++ b/src/session/session-manager-core.ts @@ -544,8 +544,10 @@ export abstract class SessionManagerCore extends EventEmitter { * its targets). Skipped entirely while a client is attending the process, * and when the window is 0 (the policy is off). Answers the closed root ids. */ - async sweepIdle(windowMs: number, options: { now?: number; attended?: boolean } = {}): Promise { - if (windowMs <= 0 || options.attended) { + async sweepIdle(windowMs: number, options: { now?: number; attended?: boolean | (() => boolean) } = {}): Promise { + const attended = (): boolean => + typeof options.attended === 'function' ? options.attended() : options.attended === true; + if (windowMs <= 0 || attended()) { return []; } const now = options.now ?? Date.now(); @@ -562,6 +564,11 @@ export abstract class SessionManagerCore extends EventEmitter { if (idleMs <= windowMs || this.idleHolds.has(root.id)) { continue; } + // Asked again right before each close: a client that arrived while an + // earlier close in this sweep ran stops the rest. + if (attended()) { + break; + } this.logger.info( `Closing idle debug session ${root.id} (idle ${Math.round(idleMs / 1000)}s): ` + `no tool call named it or its tree for ${Math.round(windowMs / 1000)}s` @@ -1221,6 +1228,8 @@ export abstract class SessionManagerCore extends EventEmitter { // A child lives in its root's scope (issue #1025, step 2). ...(parent.ownerKey !== undefined ? { ownerKey: parent.ownerKey } : {}) }); + // The tree is alive: an adoption refreshes its root's idle clock. + this.touchActivity(parent.id); child = this.sessionStore.getOrThrow(info.id); child.logDir = parent.logDir; child.effectiveBreakOnExceptions = parent.effectiveBreakOnExceptions; diff --git a/tests/core/unit/server/handlers/execution-tools.test.ts b/tests/core/unit/server/handlers/execution-tools.test.ts index 64baa540..8045922c 100644 --- a/tests/core/unit/server/handlers/execution-tools.test.ts +++ b/tests/core/unit/server/handlers/execution-tools.test.ts @@ -99,6 +99,9 @@ describe('execution tool handlers', () => { settle({ success: true, state: 'paused', data: { message: 'Paused', lastStop: { reason: 'pause', threadId: 1, timestamp: 1 } } }); await pending; expect(release).toHaveBeenCalledTimes(1); + // The wait's end is activity too: the idle clock restarts when it settles, + // not from the moment the agent asked. + expect(ctx.sessionManager.touchActivity).toHaveBeenCalledWith('test-session'); }); it('releases the hold when the wait fails', async () => { diff --git a/tests/core/unit/server/server-lifecycle.test.ts b/tests/core/unit/server/server-lifecycle.test.ts index e3714bc4..120eec9f 100644 --- a/tests/core/unit/server/server-lifecycle.test.ts +++ b/tests/core/unit/server/server-lifecycle.test.ts @@ -117,6 +117,13 @@ describe('the debug-session idle policy on the core (issue #1025, step 2)', () = vi.clearAllMocks(); }); + /** What the last sweep's attended supplier answers right now. */ + const attendedAnswer = (): boolean => { + const last = vi.mocked(mockSessionManager.sweepIdle).mock.lastCall; + const options = last?.[1] as { attended?: boolean | (() => boolean) } | undefined; + return typeof options?.attended === 'function' ? options.attended() : Boolean(options?.attended); + }; + it('start() arms a sweep at the configured default cadence and stop() clears it', async () => { debugServer = new DebugMcpServer({ debugSessionIdle: { defaultMs: 30 * 60_000 } }); expect(debugServer.idlePolicy).toEqual({ idleMs: 1_800_000, sweepMs: 60_000 }); @@ -124,7 +131,9 @@ describe('the debug-session idle policy on the core (issue #1025, step 2)', () = await debugServer.start(); await vi.advanceTimersByTimeAsync(60_000); expect(mockSessionManager.sweepIdle).toHaveBeenCalledTimes(1); - expect(mockSessionManager.sweepIdle).toHaveBeenCalledWith(1_800_000, { attended: false }); + // The supplier itself travels, so the sweep can ask again before each close. + expect(mockSessionManager.sweepIdle).toHaveBeenCalledWith(1_800_000, { attended: expect.any(Function) }); + expect(attendedAnswer()).toBe(false); await debugServer.stop(); await vi.advanceTimersByTimeAsync(180_000); @@ -189,10 +198,10 @@ describe('the debug-session idle policy on the core (issue #1025, step 2)', () = await debugServer.start(); await vi.advanceTimersByTimeAsync(60_000); - expect(mockSessionManager.sweepIdle).toHaveBeenLastCalledWith(1_800_000, { attended: true }); + expect(attendedAnswer()).toBe(true); attended = false; await vi.advanceTimersByTimeAsync(60_000); - expect(mockSessionManager.sweepIdle).toHaveBeenLastCalledWith(1_800_000, { attended: false }); + expect(attendedAnswer()).toBe(false); }); it('a sweep that rejects is logged and the next tick still runs', async () => { diff --git a/tests/core/unit/server/tool-dispatch-owner-key.test.ts b/tests/core/unit/server/tool-dispatch-owner-key.test.ts index 44d5c865..1f3733a9 100644 --- a/tests/core/unit/server/tool-dispatch-owner-key.test.ts +++ b/tests/core/unit/server/tool-dispatch-owner-key.test.ts @@ -45,6 +45,7 @@ function fakeContext(overrides: Partial = {}): ToolContext { // OWN belongs to agent-a, FOREIGN to agent-b; the anonymous scope owns nothing here. isSessionVisibleTo: vi.fn((id: string, key: string | undefined) => id === OWN ? key === 'agent-a' : id === FOREIGN ? key === 'agent-b' : false), + getSessionsVisibleTo: vi.fn(() => []), touchActivity: vi.fn() }, validateSession: (sessionId: string) => { @@ -148,6 +149,16 @@ describe('tools/call on a session outside the caller scope (issue #1025, step 2) expect(ctx.closeDebugSession).toHaveBeenCalledWith('dbg-nobody'); }); + it('a stray sessionId on a tool whose schema takes none refreshes nothing', async () => { + const ctx = fakeContext(); + const call = dispatcher(ctx); + + const result = await answer(call, 'list_debug_sessions', { sessionId: FOREIGN }, 'agent-a'); + + expect(result).toMatchObject({ success: true }); + expect(ctx.sessionManager.touchActivity).not.toHaveBeenCalled(); + }); + it('create_debug_session is never gated and stamps the caller scope on the new session', async () => { const ctx = fakeContext(); const call = dispatcher(ctx); diff --git a/tests/core/unit/session/session-idle-policy.test.ts b/tests/core/unit/session/session-idle-policy.test.ts index 72043649..9af477af 100644 --- a/tests/core/unit/session/session-idle-policy.test.ts +++ b/tests/core/unit/session/session-idle-policy.test.ts @@ -99,6 +99,32 @@ describe('SessionManager idle policy (issue #1025, step 2)', () => { expect(sessionManager.idleMsOf(childId, now() + 5000)).toBe(sessionManager.idleMsOf(rootId, now() + 5000)); }); + it('adopting a child is activity: it refreshes the root clock', async () => { + const rootId = await startRoot(); + vi.setSystemTime(now() + WINDOW - 1000); + await adoptChild(rootId); + + const closed = await sessionManager.sweepIdle(WINDOW, { now: now() + 5000 }); + + expect(closed).toEqual([]); + expect(ids()).toContain(rootId); + }); + + it('attendance is asked again before each close, so a client arriving mid-sweep stops the rest', async () => { + const a = (await sessionManager.createSession({ language: DebugLanguage.MOCK })).id; + const b = (await sessionManager.createSession({ language: DebugLanguage.MOCK })).id; + let attended = false; + vi.spyOn(sessionManager, 'closeSession').mockImplementation(async () => { + attended = true; + return true; + }); + + const closed = await sessionManager.sweepIdle(WINDOW, { now: now() + WINDOW + 1, attended: () => attended }); + + expect(closed).toHaveLength(1); + expect([a, b]).toContain(closed[0]); + }); + it('a child under an active root never expires on a clock of its own', async () => { const rootId = await startRoot(); const childId = await adoptChild(rootId); diff --git a/tests/integration/cli/http-shared-core.test.ts b/tests/integration/cli/http-shared-core.test.ts index dd950eb9..868eb921 100644 --- a/tests/integration/cli/http-shared-core.test.ts +++ b/tests/integration/cli/http-shared-core.test.ts @@ -37,6 +37,7 @@ interface Health { attended: boolean; listenStreams: number; sessions: string[]; + details: Array<{ id: string; openStreams: number }>; debugSessions: Array<{ id: string; idleMs?: number }>; } @@ -153,7 +154,15 @@ describe('Streamable HTTP: one core per process (issue #1025, step 2)', () => { // No tool call at all: presence alone, through the SDK client's standalone SSE stream. const connected = await healthUntil(port, (h) => h.attended); expect(connected.attended, JSON.stringify(connected)).toBe(true); - expect(connected.listenStreams).toBe(0); + + // Held, not a flicker: the stream is still open well after the connect + // (the SDK re-opens a dropped stream within seconds; a client that had let + // it go for good would read unattended here). + await new Promise((resolve) => setTimeout(resolve, 2_500)); + const held = await health(port); + expect(held.attended, JSON.stringify(held)).toBe(true); + expect(held.details).toHaveLength(1); + expect(held.details[0].openStreams).toBe(1); await a.client.close(); const closed = await healthUntil(port, (h) => !h.attended); diff --git a/tests/unit/cli/http-command.test.ts b/tests/unit/cli/http-command.test.ts index 2f04d7fe..ce369ab9 100644 --- a/tests/unit/cli/http-command.test.ts +++ b/tests/unit/cli/http-command.test.ts @@ -49,6 +49,8 @@ describe('HTTP Command Handler', () => { let lastTransportOptions: any; // Whether the transport mock completes an initialize handshake inside handleRequest (the SDK does). let autoInitialize = true; + // Whether the transport mock's handleRequest throws (a transport-level fault). + let handleRequestRejects = false; beforeEach(() => { mockLogger = { @@ -83,6 +85,7 @@ describe('HTTP Command Handler', () => { createdTransports = []; autoInitialize = true; + handleRequestRejects = false; MockedStreamableHTTPServerTransport.mockImplementation(function (options: any) { lastTransportOptions = options; @@ -90,7 +93,7 @@ describe('HTTP Command Handler', () => { const t: any = { // As in the SDK, the id exists only once initialize has completed. sessionId: undefined, - close: vi.fn(), + close: vi.fn().mockResolvedValue(undefined), onclose: undefined, onerror: undefined, handleRequest: vi.fn(), @@ -112,6 +115,9 @@ describe('HTTP Command Handler', () => { // initialize registers the session before the call resolves. A suite // that wants a refused handshake sets autoInitialize = false first. t.handleRequest.mockImplementation(async (_req: unknown, _res: unknown, body: any) => { + if (handleRequestRejects) { + throw new Error('transport exploded'); + } if (autoInitialize && body?.method === 'initialize') { t.triggerSessionInit(); } @@ -444,14 +450,35 @@ describe('HTTP Command Handler', () => { it('closes a transport whose initialize never completed, so nothing leaks (issue #1025)', async () => { const { app, handler } = getHandler(); autoInitialize = false; + const res = makeRes(); await handler( makeReq({ body: { jsonrpc: '2.0', id: 1, method: 'initialize', params: { protocolVersion: '2025-11-25', capabilities: {}, clientInfo: { name: 'c', version: '1' } } }, }), - makeRes() + res ); - // onsessioninitialized never fired: the SDK refused the handshake. + // onsessioninitialized never fired: the SDK refused the handshake and + // answered the client itself; our close is bookkeeping, not an error. + + expect(mockTransport.close).toHaveBeenCalledTimes(1); + expect((app as any).httpSessions.size).toBe(0); + expect(mockLogger.error).not.toHaveBeenCalled(); + expect(res.status).not.toHaveBeenCalled(); + }); + it('closes the opening transport when the handshake throws, after answering 500 (issue #1025)', async () => { + const { app, handler } = getHandler(); + autoInitialize = false; + handleRequestRejects = true; + const res = makeRes(); + await handler( + makeReq({ + body: { jsonrpc: '2.0', id: 1, method: 'initialize', params: { protocolVersion: '2025-11-25', capabilities: {}, clientInfo: { name: 'c', version: '1' } } }, + }), + res + ); + + expect(res.status).toHaveBeenCalledWith(500); expect(mockTransport.close).toHaveBeenCalledTimes(1); expect((app as any).httpSessions.size).toBe(0); }); From d34e5c73497c7cf5d0178b09c328210cbf29a2f4 Mon Sep 17 00:00:00 2001 From: JF Date: Tue, 10 Nov 2026 10:55:01 -0500 Subject: [PATCH 5/8] fix(session): nothing expires unless the operator asks, an expired session says what happened, and a session outside the caller's scope does not exist (#1025) A second look at three rulings made while building step 2, on JF's review. Idle expiry is off by default on every transport. A sessionless client gives the server no sign of life between its calls, so any finite default ends some live agent's program (an agent busy elsewhere, a human away from a paused session), while a leaked session is visible and closable. The thirty-minute http default came from the old MCP-session reaper, where an open stream protected a client; a 2026 tools-only client has no stream. The policy stays as the operator's opt-in (MCP_DEBUG_SESSION_IDLE_MS), and the per-transport default plumbing goes. The start-up log says which rule the process runs under. When the policy is on the agent can see it: list_debug_sessions rows carry idleMs (and expiresInMs), the server instructions state the window, and a tool call naming a session the policy closed answers when, under which window and whether its program was ended or released (closedBy: "idle-policy") where it used to answer "Session not found", the words a mistyped id gets. The record is bounded and told only inside the scope that owned the session. The idle log line says what the close did to the program. The adapter-cap refusal, which is where forgotten sessions now surface, says how to find and close them. The owner-key gate no longer answers for a session outside the caller's scope. Its one fixed sentence differed from the real unknown-id answer of nine tools (close, start, attach, detach, hot_reload, set and remove breakpoint, get_local_variables, evaluate) and from every tool's argument-check errors, and the operator log named the foreign session; the difference told a caller the session was there, which is the leak the seam exists to prevent. The dispatch now runs the whole call with that session hidden from the store's lookups (session-scope.ts), so each tool gives its own unknown-id answer by construction. owner-scope-contract.test.ts walks every tool that takes a sessionId, with four argument shapes and two session states, over the real SessionManager and store: 62 of 216 cases failed before, none now. Co-Authored-By: Claude Fable 5.2 --- CLAUDE.md | 2 +- README.md | 10 +- SECURITY.md | 2 +- changelog.d/1025-idle-policy.added.md | 2 +- changelog.d/1025-ownership.changed.md | 2 +- docs/architecture/session-tree.md | 19 +- docs/development/debugging-guide.md | 8 +- docs/diagnostics.md | 2 +- docs/docker-support.md | 22 +- docs/jit-diagnostics/README.md | 2 +- docs/kubernetes.md | 15 +- docs/tool-reference.md | 9 +- packages/mcp-debugger/README.md | 2 +- scripts/k8s-smoke.mjs | 2 +- src/adapters/adapter-registry.ts | 5 +- src/cli/http-command.ts | 7 - src/cli/stdio-command.ts | 5 +- src/server.ts | 28 ++- src/server/handlers/session-tools.ts | 10 + src/server/tool-context.ts | 3 + src/server/tool-dispatch.ts | 155 +++++++------- src/session/idle-policy.ts | 58 +++++- src/session/session-manager-core.ts | 47 ++++- src/session/session-scope.ts | 25 +++ src/session/session-store.ts | 8 +- src/skill-content.ts | 10 +- .../unit/server/idle-expiry-answer.test.ts | 175 ++++++++++++++++ .../unit/server/owner-scope-contract.test.ts | 192 ++++++++++++++++++ .../core/unit/server/server-lifecycle.test.ts | 61 +++--- tests/core/unit/server/server-test-helpers.ts | 1 + .../server/tool-dispatch-owner-key.test.ts | 73 ++++--- .../unit/session/session-idle-policy.test.ts | 90 +++++++- tests/unit/adapters/adapter-registry.test.ts | 5 + tests/unit/cli/http-command.test.ts | 1 - tests/unit/cli/stdio-command.test.ts | 11 +- 35 files changed, 851 insertions(+), 218 deletions(-) create mode 100644 src/session/session-scope.ts create mode 100644 tests/core/unit/server/idle-expiry-answer.test.ts create mode 100644 tests/core/unit/server/owner-scope-contract.test.ts diff --git a/CLAUDE.md b/CLAUDE.md index c9f3adda..7be61135 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -222,7 +222,7 @@ The codebase follows a **layered architecture with dependency injection** and ** - Entry point for MCP protocol communication - `src/server.ts` is the composition root and the shared core (`DebugMcpServer`: dependencies, lifecycle, session validation, breakpoint gating, the public facade methods, the `NotificationHub`); it implements `ToolContext` and passes itself to the handlers. It owns no SDK `Server`: `src/server/protocol-facade.ts` (`createProtocolServer(core, { era, deliver })`) builds one cheap SDK `Server` per connection or request over the core, and `src/server/notification-hub.ts` fans the coalesced `resources/updated` pings and `resources/list_changed` out to every facade's sink (issue #1025) - `src/server/` holds the tool schemas (`tool-schemas.ts`), argument coercion (`tool-arguments.ts`), the tools/call dispatch wrapper (`tool-dispatch.ts` over `handlers/index.ts`), argument validation (`tool-validation.ts`), result envelopes and the session-error catch helpers (`tool-result.ts`), one handler module per tool family (`handlers/*.ts`) over their shared helpers (`handlers/shared.ts`), and the resource (`output-resources.ts`) and prompt (`prompts.ts`) handlers - - Supports STDIO and Streamable HTTP transport modes. One core per process in both: on `http` every MCP session is a facade over the same debug sessions (the id is the capability; one process per agent is the isolation), a debug session outlives the MCP session that created it, and an abandoned session tree is closed by the per-debug-session idle policy (`MCP_DEBUG_SESSION_IDLE_MS`, 30 min on `http`, off on `stdio`, `0` disables; sweep `MCP_DEBUG_SESSION_IDLE_SWEEP_MS`) unless a client is attending the process (an open SSE stream). The HTTP reapers (`MCP_HTTP_STALE_SESSION_MS`, `MCP_HTTP_STREAM_LOST_SESSION_MS`) reap abandoned MCP sessions only (issue #1025) + - Supports STDIO and Streamable HTTP transport modes. One core per process in both: on `http` every MCP session is a facade over the same debug sessions (the id is the capability; one process per agent is the isolation), a debug session outlives the MCP session that created it and ends by its own close, its program ending, or the server exiting. The per-debug-session idle policy is the operator's opt-in (`MCP_DEBUG_SESSION_IDLE_MS`, off by default on every transport; sweep `MCP_DEBUG_SESSION_IDLE_SWEEP_MS`): when set, an abandoned session tree is closed unless a client is attending the process (an open SSE stream), `list_debug_sessions` rows show `idleMs`/`expiresInMs`, and a call naming a session the policy closed answers `closedBy: "idle-policy"`. A session outside the caller's owner scope is hidden from the call's lookups (`src/session/session-scope.ts`), so every tool answers it as an unknown id. The HTTP reapers (`MCP_HTTP_STALE_SESSION_MS`, `MCP_HTTP_STREAM_LOST_SESSION_MS`) reap abandoned MCP sessions only (issue #1025) - Dynamically discovers available language adapters 2. **Adapter System** (NEW) diff --git a/README.md b/README.md index c9165414..ec838374 100644 --- a/README.md +++ b/README.md @@ -228,9 +228,13 @@ One `http` process is one debugger: every MCP client of it sees the same debug s session is addressable by whoever holds its id (the handle is the capability — run one process per agent if agents must not see each other's work). A debug session outlives the MCP session that created it, so a client that reconnects can list and resume or close what it left behind. -What ends an abandoned one is the idle policy: a session tree that no tool call has named for -30 minutes (`MCP_DEBUG_SESSION_IDLE_MS`; `0` disables) is closed, unless a client is attending -the process — while any client holds its SSE stream open, nothing expires. Abandoned MCP +A debug session ends by its own close, by its program ending, or with the server: nothing expires +on a timer by default, so a quiet agent never loses a paused program. A server that is shared or +long-lived, or that attaches to something that must not stay paused, can turn the idle policy on: +with `MCP_DEBUG_SESSION_IDLE_MS` set, a session tree that no tool call has named for that long is +closed (a launched program is ended, an attached one released), unless a client is attending the +process — while any client holds its SSE stream open, nothing expires. `list_debug_sessions` +shows each session's idle time, so a forgotten one is easy to find and close. Abandoned MCP sessions themselves are reaped separately (`MCP_HTTP_STREAM_LOST_SESSION_MS`, `MCP_HTTP_STALE_SESSION_MS`). `GET /health` lists the MCP sessions, the debug sessions with their idle time, and whether anyone is attending. diff --git a/SECURITY.md b/SECURITY.md index 59093078..df167c27 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -68,7 +68,7 @@ If you want to constrain what the MCP client can reach, constrain the mcp-debugg - **Containers.** Run the published `debugmcp/mcp-debugger` image with the workspace mounted at `/workspace`. The container boundary is the filesystem boundary. - **OS permissions.** Run mcp-debugger as a user with read access only to the source you want it to debug, and no access to secrets, SSH keys, credentials, etc. The kernel enforces the limits. -- **Network exposure.** If running in http mode, treat the port as a remote shell endpoint — anyone who can reach it inherits the mcp-debugger process's privileges. Bind to loopback or put it behind authenticated network access. One http process is single-tenant: every client of it sees and can drive every debug session (a session id is the capability); run one process per agent where that matters. The server is built to scope sessions per principal once it verifies callers itself, but no authenticated http mode ships yet, and an authenticating reverse proxy in front of `/mcp` does not identify callers to the server — every caller still shares one scope. +- **Network exposure.** If running in http mode, treat the port as a remote shell endpoint — anyone who can reach it inherits the mcp-debugger process's privileges. Bind to loopback or put it behind authenticated network access. One http process is single-tenant: every client of it sees and can drive every debug session (a session id is the capability); run one process per agent where that matters. The server is built to scope sessions per principal once it verifies callers itself, but no authenticated http mode ships yet, and an authenticating reverse proxy in front of `/mcp` does not identify callers to the server — every caller still shares one scope. Nothing closes a debug session on a timer by default: on a shared or long-lived server set `MCP_DEBUG_SESSION_IDLE_MS`, so a client that vanishes cannot leave an attached process paused until the server exits. - **Remote debuggees.** When using `attach_to_process` to debug a process on another host, the MCP client inherits that debuggee's privileges too. Apply the same containment reasoning there. - **MCP client trust.** Do not expose mcp-debugger to an MCP client that you trust less than the OS user mcp-debugger runs under. - **DAP mirror endpoints.** `expose_session` opens a per-session DAP endpoint bound to `127.0.0.1` and gated by a random per-expose token. The mirror rejects execution control and breakpoint changes, but it forwards `evaluate` — and DAP evaluate can execute arbitrary code in the debuggee — so **treat the mirror token as a debuggee-execution capability, not a view-only credential**. Any local process that obtains the token gets that capability; the token is returned only in the `expose_session` result and is redacted from logs. "Read-only" means control is rejected, not that the debuggee is immutable. diff --git a/changelog.d/1025-idle-policy.added.md b/changelog.d/1025-idle-policy.added.md index 94430aea..3cf88373 100644 --- a/changelog.d/1025-idle-policy.added.md +++ b/changelog.d/1025-idle-policy.added.md @@ -1 +1 @@ -**A per-debug-session idle policy replaces the MCP-session reaper as what ends an abandoned debug session.** On `http`, a debug-session tree that no tool call has named for `MCP_DEBUG_SESSION_IDLE_MS` (default 30 minutes; `0` disables) is closed through the normal close cascade — a launch root closes its subtree, an attach root releases its targets — unless a client is attending the process (an open SSE stream), in which case nothing expires; a tool call naming any session in a tree refreshes the whole tree, an in-flight `wait_for_stop` holds it, and children never expire on a clock of their own. The sweep runs every `MCP_DEBUG_SESSION_IDLE_SWEEP_MS` (default 60 s). On `stdio` the pipe is the lifetime and the policy defaults to off (the same variable turns it on). The HTTP reapers (`MCP_HTTP_STALE_SESSION_MS`, `MCP_HTTP_STREAM_LOST_SESSION_MS`) still reap abandoned MCP sessions, but reaping one no longer closes any debug session; a crashed client's sessions end when the idle window runs out, or sooner by an explicit close or the debuggee's exit. Step 2 of #1025. +**An opt-in per-debug-session idle policy; nothing expires by default.** A debug session ends by its own close, its program ending, or the server exiting, on every transport — no timer ends one unless the operator asks. With `MCP_DEBUG_SESSION_IDLE_MS` set, a debug-session tree that no tool call has named for that long is closed through the normal close cascade — a launch root closes its subtree and ends its program, an attach root releases its targets — unless a client is attending the process (an open SSE stream), in which case nothing expires; a tool call naming any session in a tree refreshes the whole tree, an in-flight `wait_for_stop` holds it, and children never expire on a clock of their own. The sweep runs every `MCP_DEBUG_SESSION_IDLE_SWEEP_MS` (default 60 s). The agent can see it: `list_debug_sessions` rows carry `idleMs` (and `expiresInMs` when a policy is on), a tool call naming a session the policy closed answers what happened — when, under which window, and whether its program was ended or released (`closedBy: "idle-policy"`) — instead of "Session not found", and the server instructions mention the policy when it is on. The HTTP reapers (`MCP_HTTP_STALE_SESSION_MS`, `MCP_HTTP_STREAM_LOST_SESSION_MS`) still reap abandoned MCP sessions, but reaping one no longer closes any debug session: **a crashed client's debug sessions stay until someone closes them, the idle window (if set) runs out, or the server exits.** Set the variable on a shared or long-lived `http` server, and wherever an attached process must not stay paused (the Kubernetes recipe sets 5 minutes). Step 2 of #1025. diff --git a/changelog.d/1025-ownership.changed.md b/changelog.d/1025-ownership.changed.md index ac016155..38055808 100644 --- a/changelog.d/1025-ownership.changed.md +++ b/changelog.d/1025-ownership.changed.md @@ -1 +1 @@ -**One debugger core per `http` process: every MCP client sees the same debug sessions.** A Streamable HTTP server used to build an isolated server per MCP client session, so `list_debug_sessions` listed only the caller's own sessions and another client's id was refused with "belongs to another MCP client session" (#939). The 2026-07-28 revision of the protocol has no client sessions to key that on, so the model is now the one the spec names — the handle is the capability: a debug session is addressable by whoever holds its id, `list_debug_sessions` is the whole process's list, and several agents against one `http` process see each other's sessions (one process per agent is the isolation, as `stdio` already gives). Debug sessions therefore outlive the MCP session that created them, and a client that reconnects can list and resume or close what it left behind. The seam for real isolation is in place: a session is created in its caller's scope — the authenticated principal when the request carried one, otherwise the shared anonymous scope — and an out-of-scope id answers as unknown; no authenticated `http` mode ships yet (an authenticating reverse proxy does not identify callers to the server), so every caller is anonymous and sees everything, which is today's single-tenant behaviour. `GET /health`'s `details[]` no longer carries a per-client `debugSessions` list; the process-wide `debugSessions` array replaces it. `GET /health` now lists the process's debug sessions beside the MCP sessions, and says whether a client is attending. The ten-per-language adapter cap is per process now, as it always was on `stdio`. Step 2 of #1025. +**One debugger core per `http` process: every MCP client sees the same debug sessions.** A Streamable HTTP server used to build an isolated server per MCP client session, so `list_debug_sessions` listed only the caller's own sessions and another client's id was refused with "belongs to another MCP client session" (#939). The 2026-07-28 revision of the protocol has no client sessions to key that on, so the model is now the one the spec names — the handle is the capability: a debug session is addressable by whoever holds its id, `list_debug_sessions` is the whole process's list, and several agents against one `http` process see each other's sessions (one process per agent is the isolation, as `stdio` already gives). Debug sessions therefore outlive the MCP session that created them, and a client that reconnects can list and resume or close what it left behind. The seam for real isolation is in place: a session is created in its caller's scope — the authenticated principal when the request carried one, otherwise the shared anonymous scope — and a session outside the caller's scope does not exist for that caller — every tool answers it exactly as it answers an id nobody holds; no authenticated `http` mode ships yet (an authenticating reverse proxy does not identify callers to the server), so every caller is anonymous and sees everything, which is today's single-tenant behaviour. `GET /health`'s `details[]` no longer carries a per-client `debugSessions` list; the process-wide `debugSessions` array replaces it. `GET /health` now lists the process's debug sessions beside the MCP sessions, and says whether a client is attending. The ten-per-language adapter cap is per process now, as it always was on `stdio`, and its refusal says how to find and close the sessions holding the slots. Step 2 of #1025. diff --git a/docs/architecture/session-tree.md b/docs/architecture/session-tree.md index 88c4f783..e4cdff70 100644 --- a/docs/architecture/session-tree.md +++ b/docs/architecture/session-tree.md @@ -472,10 +472,19 @@ Two guardrails: contract, and went with the one-core-per-process change (shipped 2026-11, #1025 step 2). The seam for real isolation is in place: a session is created in its caller's scope — the authenticated principal when the request carried one, otherwise the shared anonymous scope — - and an out-of-scope id answers as unknown; with no authentication every caller is anonymous. -2. Lifetime without an MCP session is a **per-debug-session idle policy**: - `MCP_DEBUG_SESSION_IDLE_MS`, default thirty minutes on `http` and off on `stdio` (the pipe is - the lifetime), `0` disables; the sweep runs every `MCP_DEBUG_SESSION_IDLE_SWEEP_MS` (60 s). + and a session outside the caller's scope does not exist for that caller: the dispatch hides it + from the call's lookups, so every tool answers it exactly as it answers an id nobody holds + (pinned tool by tool in `owner-scope-contract.test.ts`); with no authentication every caller is + anonymous. +2. Lifetime without an MCP session: a debug session ends by its own close, its program ending, or + the server exiting. A **per-debug-session idle policy** exists and is **off by default on every + transport** (decided with JF 2026-11-10, replacing the thirty-minute `http` default this section + first carried: a sessionless client gives the server no sign of life between its calls, so any + finite default ends some live agent's program, while a leaked session is visible in + `list_debug_sessions` and closable by anyone). An operator of a shared or long-lived server + turns it on with `MCP_DEBUG_SESSION_IDLE_MS`; the sweep runs every + `MCP_DEBUG_SESSION_IDLE_SWEEP_MS` (60 s). When it is on, a session's row shows `expiresInMs`, + and a later call naming a session the policy closed answers what happened, in its owner's scope. Only roots have a clock: a tool call naming any session in a tree refreshes the tree's root, and a child never expires on a clock of its own — the tree goes with its root (decided 2026-11-09, amending the per-descendant clocks this section first proposed: a worker paused under a parent @@ -560,7 +569,7 @@ Taken by the author of this document, marked so they can be overturned: - The child's connection lives in the parent's worker (the constraints above). - The per-tree cap defaults to 32 and is named `DEBUG_MCP_MAX_CHILD_SESSIONS`; `0` disables child-session adoption, excluding js-debug's mandatory first target. -- The idle policy knob is named `MCP_DEBUG_SESSION_IDLE_MS` (sweep: `MCP_DEBUG_SESSION_IDLE_SWEEP_MS`). +- The idle policy knob is named `MCP_DEBUG_SESSION_IDLE_MS` (sweep: `MCP_DEBUG_SESSION_IDLE_SWEEP_MS`); it is off unless set (decided with JF, 2026-11-10). - Only roots have an idle clock: activity on any session in a tree refreshes its root, a child never expires alone, a tree-aware wait holds the root for the duration of the call, and the sweep is skipped while a client attends the process (decided with JF, 2026-11-09). diff --git a/docs/development/debugging-guide.md b/docs/development/debugging-guide.md index 5984dfb6..dd134153 100644 --- a/docs/development/debugging-guide.md +++ b/docs/development/debugging-guide.md @@ -96,10 +96,10 @@ connection active while the nested server is paused. Debug sessions on a Streamable HTTP server belong to the process, not to the MCP client session that created them (one `DebugMcpServer` core per process, one SDK `Server` facade per MCP session; issue #1025). A one-shot driver script that opens a fresh MCP session per run reaches -the debug session its previous run created by id, and `list_debug_sessions` lists it. What ends -an abandoned one is the idle policy: a session tree no tool call has named for -`MCP_DEBUG_SESSION_IDLE_MS` (default 30 min; `0` disables) is closed, unless a client is attending -the process — while any client holds its SSE stream open, nothing expires. The abandoned MCP +the debug session its previous run created by id, and `list_debug_sessions` lists it. Nothing +ends a debug session on a timer by default; with `MCP_DEBUG_SESSION_IDLE_MS` set, a session tree +no tool call has named for that long is closed, unless a client is attending the process — while +any client holds its SSE stream open, nothing expires. The abandoned MCP sessions themselves are reaped separately (`MCP_HTTP_STREAM_LOST_SESSION_MS`, default 2 min after the stream dropped; `MCP_HTTP_STALE_SESSION_MS`, default 30 min for a pure-POST client). diff --git a/docs/diagnostics.md b/docs/diagnostics.md index 12abfc0b..ad0c9a25 100644 --- a/docs/diagnostics.md +++ b/docs/diagnostics.md @@ -251,7 +251,7 @@ The runtime-affecting variables the server and its adapters read (the [developme | `MCP_HTTP_STALE_SESSION_MS` | Idle time before an HTTP session that never opened an SSE stream is reaped (default `1800000`; `0` disables that path) | | `MCP_HTTP_STREAM_LOST_SESSION_MS` | Idle time before an HTTP session whose SSE stream dropped and never returned is reaped (default `120000`; `0` falls back to the stale window; issue #658). Reaping an MCP session closes no debug session (see `MCP_DEBUG_SESSION_IDLE_MS`) | | `MCP_HTTP_STALE_SWEEP_INTERVAL_MS` | How often that reap sweep runs (default `60000`) | -| `MCP_DEBUG_SESSION_IDLE_MS` | Idle window after which a debug-session tree no tool call has named is closed, unless a client is attending the process — a crashed client's sessions, and any paused attach target they hold, are released this soon after its stream dropped (default `1800000` on `http`, `0` = off on `stdio`; `0` disables; issue #1025) | +| `MCP_DEBUG_SESSION_IDLE_MS` | Opt-in idle window, off by default on every transport (unset or `0`): a debug session then lives until it is closed, its program ends, or the server exits. When set, a debug-session tree no tool call has named for this long is closed — a launched program ended, an attached target released — unless a client is attending the process. Set it on a shared or long-lived `http` server so a crashed client's sessions, and any paused attach target they hold, are released (issue #1025) | | `MCP_DEBUG_SESSION_IDLE_SWEEP_MS` | How often that idle sweep runs (default `60000`) | | `MCP_HTTP_ALLOWED_HOSTS` | Comma-separated `Host`/`Origin` hostnames the `http` endpoint accepts besides the loopback trio (same as repeating `--allowed-host`); an unusable entry stops startup with the entry named | | `DAP_TRACE` / `DAP_TRACE_FILE` | Enable per-session DAP frame capture / choose its file | diff --git a/docs/docker-support.md b/docs/docker-support.md index 20823379..1671d276 100644 --- a/docs/docker-support.md +++ b/docs/docker-support.md @@ -258,25 +258,25 @@ may reach it. All are passed through with `-e`: (or omit the variable) for detached network-mode containers. It is unrelated to the `stdio`-mode exit policy described under [Troubleshooting](#common-docker-issues), which is always on and needs no variable. -- **`MCP_DEBUG_SESSION_IDLE_MS`** (`src/session/idle-policy.ts`) -- the idle window after which - a debug-session tree that no tool call has named is closed, releasing any ptrace claim on the - target, unless a client is attending the process (an open SSE stream). Defaults to 30 minutes - (1800000) in `http` mode and off in `stdio` mode, where the pipe is the lifetime; `0` disables; - invalid values log a warning and fall back to the default. `MCP_DEBUG_SESSION_IDLE_SWEEP_MS` - (default 60000) is the sweep cadence. A short-lived diagnostic container wants a much tighter - window -- see the [Kubernetes debugging recipe](kubernetes.md), which sets it to 5 minutes - (issue #1025). +- **`MCP_DEBUG_SESSION_IDLE_MS`** (`src/session/idle-policy.ts`) -- opt-in, off by default on + every transport (unset or `0`): a debug session then lives until it is closed, its program + ends, or the container exits. When set, a debug-session tree that no tool call has named for + that long is closed, releasing any ptrace claim on the target, unless a client is attending + the process (an open SSE stream). Invalid values log a warning and leave it off. + `MCP_DEBUG_SESSION_IDLE_SWEEP_MS` (default 60000) is the sweep cadence. Set it on a + long-lived or shared container; a short-lived diagnostic one wants a tight window -- see + the [Kubernetes debugging recipe](kubernetes.md), which sets it to 5 minutes (issue #1025). - **`MCP_HTTP_STALE_SESSION_MS`** (`src/cli/http-command.ts`) -- idle window before an HTTP MCP session with no open SSE stream is reaped. Defaults to 30 minutes (1800000); `0` disables that path; invalid values log a warning and fall back to the default. Read only by `http` - mode. Reaping an MCP session closes no debug session: those belong to the process and answer - to `MCP_DEBUG_SESSION_IDLE_MS`. + mode. Reaping an MCP session closes no debug session: those belong to the process, and only + `MCP_DEBUG_SESSION_IDLE_MS`, when set, ends one on a timer. - **`MCP_HTTP_STREAM_LOST_SESSION_MS`** -- the short window for an MCP session whose SSE stream dropped and never came back (default 2 minutes; `0` falls back to the stale window). An MCP SDK client keeps a GET stream open, so when it crashes or exits without `DELETE /mcp` the server sees the socket close and forgets the connection within this window plus one sweep (issue #658); the moment that stream closes the process is also no longer attended, so - the idle policy can end the debug sessions it left behind. `GET /health` lists the MCP + the idle policy, when it is set, can end the debug sessions it left behind. `GET /health` lists the MCP sessions, the debug sessions and whether anyone is attending. - **`MCP_HTTP_ALLOWED_HOSTS`** (`src/cli/host-allowlist.ts`) -- comma-separated `Host` header values to accept in addition to the loopback trio (`localhost`, `127.0.0.1`, `[::1]`); the same diff --git a/docs/jit-diagnostics/README.md b/docs/jit-diagnostics/README.md index 59d392bd..d298de66 100644 --- a/docs/jit-diagnostics/README.md +++ b/docs/jit-diagnostics/README.md @@ -128,7 +128,7 @@ kubectl port-forward "pod/$POD" 3001:3001 & Register the endpoint with your agent (e.g. `claude mcp add-json sick-pod '{"type":"http","url":"http://127.0.0.1:3001/mcp"}'`). -The HTTP server closes debug sessions abandoned by a crashed client, releasing any ptrace claim on the target, through the per-debug-session idle policy: a session tree no tool call has named for 30 minutes is closed unless a client is attending the process (`MCP_DEBUG_SESSION_IDLE_MS`; `0` disables). For a short-lived diagnostic sidecar a tighter window is safer — add `--env=MCP_DEBUG_SESSION_IDLE_MS=300000` (5 minutes) to the `kubectl debug` command. The abandoned MCP sessions themselves are reaped separately: a client whose SSE stream dropped and never returned (any MCP SDK client that crashed or exited without `DELETE`) is reaped after 2 minutes idle (`MCP_HTTP_STREAM_LOST_SESSION_MS`), one that never opened a stream after 30 (`MCP_HTTP_STALE_SESSION_MS`). For a short-lived diagnostic sidecar a tighter stale window is safer — add `--env=MCP_HTTP_STALE_SESSION_MS=300000` (5 minutes; `0` disables) to the `kubectl debug` command. `GET /health` on the sidecar lists the process's debug sessions with their idle time, and whether a client is attending. +The HTTP server closes nothing on a timer by default, so a debug session abandoned by a crashed client would keep its ptrace claim on the target until the sidecar exits. For a diagnostic sidecar turn the per-debug-session idle policy on — add `--env=MCP_DEBUG_SESSION_IDLE_MS=300000` (5 minutes) to the `kubectl debug` command: a session tree no tool call has named for that long is then closed, releasing the claim, unless a client is attending the process. The abandoned MCP sessions themselves are reaped separately: a client whose SSE stream dropped and never returned (any MCP SDK client that crashed or exited without `DELETE`) is reaped after 2 minutes idle (`MCP_HTTP_STREAM_LOST_SESSION_MS`), one that never opened a stream after 30 (`MCP_HTTP_STALE_SESSION_MS`). For a short-lived diagnostic sidecar a tighter stale window is safer — add `--env=MCP_HTTP_STALE_SESSION_MS=300000` (5 minutes; `0` disables) to the `kubectl debug` command. `GET /health` on the sidecar lists the process's debug sessions with their idle time, and whether a client is attending. Facts that will save you an afternoon (all observed, not assumed): diff --git a/docs/kubernetes.md b/docs/kubernetes.md index a7a15ec3..bb0b6c39 100644 --- a/docs/kubernetes.md +++ b/docs/kubernetes.md @@ -144,10 +144,11 @@ The flags that matter — each one is load-bearing: pass `-- /app/entry.sh http -p 3001` — bare `-- http -p 3001` fails with `exec: "http": executable file not found`. (Git Bash on Windows mangles `/app/...` — prefix the command with `MSYS_NO_PATHCONV=1`.) -- **`MCP_DEBUG_SESSION_IDLE_MS=300000`** tightens the idle policy that closes a - debug session nobody has named from 30 min to 5 — releasing the ptrace claim - on your pod sooner once the client is gone (while a client holds its SSE - stream open, nothing expires). **`MCP_HTTP_STALE_SESSION_MS=300000`** tightens +- **`MCP_DEBUG_SESSION_IDLE_MS=300000`** turns the idle policy on at 5 minutes: a + debug session nobody has named for that long is closed — releasing the ptrace + claim on your pod once the client is gone (while a client holds its SSE stream + open, nothing expires). Leave it out and nothing expires: a crashed client's + attach keeps its claim until the sidecar exits. **`MCP_HTTP_STALE_SESSION_MS=300000`** tightens the separate reaper for abandoned MCP sessions the same way. (A crashed SDK client's MCP session, whose stream the server saw close, is reaped after 2 min regardless — `MCP_HTTP_STREAM_LOST_SESSION_MS`; its debug sessions then @@ -224,9 +225,9 @@ the MCP endpoint, and the containerized server attaches to python, ruby logpoints (`set_breakpoint` with `logMessage`): the pod keeps running at full speed while values stream to `get_output`. - An abandoned attach no longer leaves an orphaned tracer (#337): teardown - kills the adapter's process group, and the sidecar's idle policy closes a - crashed client's debug sessions once nobody attends the process - (`MCP_DEBUG_SESSION_IDLE_MS`). Suspect a leftover anyway? + kills the adapter's process group, and the sidecar's idle policy — on because + the recipe sets `MCP_DEBUG_SESSION_IDLE_MS` — closes a crashed client's debug + sessions once nobody attends the process. Suspect a leftover anyway? `grep TracerPid /proc/1/status` from the sidecar. ## Distroless and minimal images diff --git a/docs/tool-reference.md b/docs/tool-reference.md index c82d78f0..5f4ebc2f 100644 --- a/docs/tool-reference.md +++ b/docs/tool-reference.md @@ -87,7 +87,7 @@ Creates a new debugging session. **Notes:** - Session IDs are UUIDs in the format `xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx` - Sessions start in `"created"` state -- **One debugger core per process; the id is the capability (issue #1025).** On Streamable HTTP every MCP client of a process sees the same debug sessions: `list_debug_sessions` is the whole process's list, and any client holding an id can drive or close that session — run one `http` process per agent where agents must not see each other's work. A debug session outlives the MCP session that created it, so a one-shot script that opens a fresh MCP session per call reaches the sessions its previous run created, and a client that reconnects can resume or close what it left behind. What ends an abandoned session is the **idle policy**: a session tree that no tool call has named for `MCP_DEBUG_SESSION_IDLE_MS` (default 30 minutes on `http`, off on `stdio`; `0` disables) is closed through the normal close cascade, unless a client is attending the process — while any client holds its SSE stream open, nothing expires; an in-flight `wait_for_stop` holds its tree; children never expire on a clock of their own. Explicit close and debuggee exit end a session sooner, as always. The server is built to scope sessions per principal once it verifies callers itself (no authenticated `http` mode ships yet; an authenticating reverse proxy does not identify callers to the server), so today every caller is in the one shared scope +- **One debugger core per process; the id is the capability (issue #1025).** On Streamable HTTP every MCP client of a process sees the same debug sessions: `list_debug_sessions` is the whole process's list, and any client holding an id can drive or close that session — run one `http` process per agent where agents must not see each other's work. A debug session outlives the MCP session that created it, so a one-shot script that opens a fresh MCP session per call reaches the sessions its previous run created, and a client that reconnects can resume or close what it left behind. A session ends by `close_debug_session`, by its program ending, or with the server: nothing expires on a timer by default. An operator can turn on the **idle policy** (`MCP_DEBUG_SESSION_IDLE_MS`, off by default on every transport): a session tree that no tool call has named for that long is then closed through the normal close cascade (a launched program is ended, an attached one released), unless a client is attending the process — while any client holds its SSE stream open, nothing expires; an in-flight `wait_for_stop` holds its tree; children never expire on a clock of their own. `list_debug_sessions` shows each session's `idleMs` and, with a policy on, `expiresInMs`; a later call naming a session the policy closed answers what happened, with `closedBy: "idle-policy"`. The server is built to scope sessions per principal once it verifies callers itself (no authenticated `http` mode ships yet; an authenticating reverse proxy does not identify callers to the server), so today every caller is in the one shared scope - When `port`, `processId` or an `adapterConfig` URI/info-file key is provided in `create_debug_session`, the server performs an inline attach (creating the session and immediately attaching). The session exists before the addressing gate can refuse the form (a `port` on C/C++, say), so such a refusal answers `success: false` with a usable `sessionId`. The response then mirrors `attach_to_process`: alongside `sessionId` it carries `state`, the attach `data` payload, an optional `warning`, and — when a requested post-attach pause has not landed yet — `pending: true`, with `message` saying so (`…; post-attach pause pending — the target stops when it next executes code; call wait_for_stop to block until it does (pass stopOnEntry: false to attach without pausing)`) --- @@ -109,7 +109,8 @@ Lists all active debugging sessions — the whole process's list, whichever MCP "language": "python", "state": "created", "createdAt": "2025-06-11T04:53:14.762Z", - "updatedAt": "2025-06-11T04:53:14.762Z" + "updatedAt": "2025-06-11T04:53:14.762Z", + "idleMs": 4210 } ], "count": 1 @@ -131,6 +132,8 @@ Lists all active debugging sessions — the whole process's list, whichever MCP Errored sessions include optional `diagnostics` with the current launch attempt's server-host `proxyLogPath` and remote-safe `proxyLogResource`. The record is retained for proxy initialization failures and for proxy/adapter deaths after initialization, and is cleared when a new launch or attach attempt begins. +**`idleMs`:** milliseconds since a tool call last named this session or any session in its tree (a child reports its root's) — what tells a forgotten session from a live one. **`expiresInMs`:** present only when the operator turned the idle policy on (`MCP_DEBUG_SESSION_IDLE_MS`): how long the tree has before the policy closes it; any tool call naming the session resets it, a `wait_for_stop` in flight holds it, and nothing expires while a client is attending the process. + **`parentSessionId`:** present on child rows only. A child session is created by the server, never by `create_debug_session`, when the adapter reports a process the debuggee started: a Python launch or attach — debugpy's sub-process debugging is on by default; `dapLaunchArgs: { subProcess: false }` opts a launch out and an attach target decides for itself with `--configure-subProcess` — where debugpy reports each child, or a JavaScript launch or verified-local attach — js-debug's `autoAttachChildProcesses` is on by default for launch or an inspector target that can read both a temporary server-side probe file and the installed js-debug bootloader files; other attaches default it off with a warning; `dapLaunchArgs: { autoAttachChildProcesses: false }` opts a launch out, `adapterConfig: { autoAttachChildProcesses: false }` an attach — where js-debug reports every Node process the program forks or spawns, `cluster` workers and grandchildren included (see [the session tree](./architecture/session-tree.md)). The child inherits the parent's `language`, its `name` is the adapter's own label for the process (debugpy: `Subprocess `; js-debug: `