diff --git a/src/mcplocal/src/http/project-mcp-endpoint.ts b/src/mcplocal/src/http/project-mcp-endpoint.ts index 7f64636..8b298de 100644 --- a/src/mcplocal/src/http/project-mcp-endpoint.ts +++ b/src/mcplocal/src/http/project-mcp-endpoint.ts @@ -30,6 +30,8 @@ import { AuditCollector } from '../audit/collector.js'; import { WireNameCodec, routeWithWireNames } from '../util/wire-names.js'; import { newTraceCode } from '../util/trace-code.js'; import { runInRequestScope } from '../request-context.js'; +import { adoptSession } from './session-adopt.js'; +import { degradationNotice } from '../util/degrade.js'; interface ProjectCacheEntry { router: McpRouter; @@ -48,6 +50,12 @@ export function registerProjectMcpEndpoint(app: FastifyInstance, mcpdClient: Mcp let resolvedUserName: string | null | undefined; // undefined = not yet resolved const projectCache = new Map(); const sessions = new Map(); + /** + * Sessions recreated after a restart, awaiting their one-shot ⚠ notice. + * Delivered on the next tools/call result — the only response a model + * reliably reads as prose. + */ + const pendingRecreationNotice = new Set(); // Wire-name codecs are keyed per project and OUTLIVE the router cache TTL: // a client may call a tool it listed minutes ago through a refreshed router, // and the mapping must still resolve. @@ -217,6 +225,27 @@ export function registerProjectMcpEndpoint(app: FastifyInstance, mcpdClient: Mcp } // POST /projects/:projectName/mcp — JSON-RPC requests + /** + * Prepend the recreation notice to the first tool result after a session was + * rebuilt. Silent recreation would hand an agent an ungated tool list with no + * signal that its gate state had vanished. + */ + function maybeAnnotateRecreation(response: unknown, sid: string, method?: string): unknown { + if (method !== 'tools/call' || !pendingRecreationNotice.has(sid)) return response; + pendingRecreationNotice.delete(sid); + + const r = response as { result?: { content?: Array<{ type?: string; text?: string }> } }; + const first = r.result?.content?.[0]; + if (!first || first.type !== 'text' || typeof first.text !== 'string') return response; + + first.text = degradationNotice( + 'Session state', + `mcplocal restarted; session ${sid.slice(0, 8)} was recreated`, + 'Gate state and cached results were lost — if an expected tool is missing, call begin_session again.', + ) + first.text; + return response; + } + app.post<{ Params: { projectName: string } }>('/projects/:projectName/mcp', async (request, reply) => { const { projectName } = request.params; const sessionId = request.headers['mcp-session-id'] as string | undefined; @@ -229,10 +258,15 @@ export function registerProjectMcpEndpoint(app: FastifyInstance, mcpdClient: Mcp return; } - if (sessionId && !sessions.has(sessionId)) { - reply.code(404).send({ error: 'Session not found' }); - return; - } + // An unknown session id means mcplocal restarted (or the session was + // evicted) while the client still holds a perfectly good id. Replying 404 + // is a dead end: it lands OUTSIDE the JSON-RPC envelope with no id, so the + // client cannot correlate it and stalls until its own timeout — 3.5ms + // server-side became 1800s client-side. Recreate instead, keep the id the + // client already has, and tell the model what it lost. + const recreatingSessionId = sessionId !== undefined && !sessions.has(sessionId) + ? sessionId + : undefined; // New session — get/create project router let router: McpRouter; @@ -243,9 +277,11 @@ export function registerProjectMcpEndpoint(app: FastifyInstance, mcpdClient: Mcp return; } - const transport = new StreamableHTTPServerTransport({ - sessionIdGenerator: () => randomUUID(), - onsessioninitialized: (id) => { + // Called by onsessioninitialized on the normal path, and explicitly on the + // recreation path — where onsessioninitialized never fires. Missing any of + // this on the adopt path would give recreated sessions null userName on + // every audit event. + const registerSession = (id: string): void => { sessions.set(id, { transport, projectName }); trafficCapture?.emit({ timestamp: new Date().toISOString(), @@ -283,7 +319,11 @@ export function registerProjectMcpEndpoint(app: FastifyInstance, mcpdClient: Mcp verified: true, payload: { projectName }, }); - }, + }; + + const transport = new StreamableHTTPServerTransport({ + sessionIdGenerator: () => randomUUID(), + onsessioninitialized: (id) => { registerSession(id); }, }); // Per-request correlationId map for linking client ↔ upstream event pairs. @@ -395,7 +435,9 @@ export function registerProjectMcpEndpoint(app: FastifyInstance, mcpdClient: Mcp }); requestCorrelations.delete(requestId); - await transport.send(response as unknown as JSONRPCMessage); + await transport.send( + maybeAnnotateRecreation(response, sid, method) as unknown as JSONRPCMessage, + ); } }; @@ -414,6 +456,19 @@ export function registerProjectMcpEndpoint(app: FastifyInstance, mcpdClient: Mcp } }; + if (recreatingSessionId !== undefined) { + // Adopt the client's existing id rather than minting a new one: a + // re-handshake would invalidate everything the client has cached about + // this session. + adoptSession(transport, recreatingSessionId); + registerSession(recreatingSessionId); + pendingRecreationNotice.add(recreatingSessionId); + console.error( + `[mcp] recreated session ${recreatingSessionId.slice(0, 8)} for project '${projectName}' ` + + '(mcplocal restarted or the session was evicted); gate state was lost', + ); + } + await transport.handleRequest(request.raw, reply.raw, request.body); reply.hijack(); }); diff --git a/src/mcplocal/src/http/server.ts b/src/mcplocal/src/http/server.ts index 56a0210..21219a6 100644 --- a/src/mcplocal/src/http/server.ts +++ b/src/mcplocal/src/http/server.ts @@ -42,6 +42,24 @@ export async function createHttpServer( methods: ['GET', 'POST', 'PUT', 'DELETE', 'PATCH'], }); + // MCP clients close a session with DELETE + `Content-Type: application/json` + // and an EMPTY body. Fastify's default JSON parser rejects that with + // FST_ERR_CTP_EMPTY_JSON_BODY before the route handler ever runs, so + // sessions.delete() never happened and every "closed" session leaked -- + // which is a direct producer of the stale-session condition that used to + // 404. Treat an empty body as no body. + app.addContentTypeParser('application/json', { parseAs: 'string' }, (_req, body, done) => { + if (body === '' || body === undefined) { + done(null, undefined); + return; + } + try { + done(null, JSON.parse(body as string) as unknown); + } catch (err) { + done(err as Error, undefined); + } + }); + // Health endpoint app.get('/health', async (_request, reply) => { const upstreams = deps.router.getUpstreamNames(); diff --git a/src/mcplocal/src/http/session-adopt.ts b/src/mcplocal/src/http/session-adopt.ts new file mode 100644 index 0000000..336370b --- /dev/null +++ b/src/mcplocal/src/http/session-adopt.ts @@ -0,0 +1,47 @@ +/** + * Let a freshly-built transport adopt a session ID the client already holds. + * + * mcplocal keeps MCP sessions in memory only, so any restart — a deploy, an RPM + * install, `scripts/release.sh` — invalidates every live client's session id. + * The old behaviour was a bare `404 {"error":"Session not found"}` outside the + * JSON-RPC envelope: 3.5ms server-side, but the client cannot correlate it and + * stalls until its own timeout (1800s in Claude Code). Recreating the session + * is strictly better, and keeping the client's EXISTING id is the point — a + * re-handshake would change it and every assumption the client has cached. + * + * The SDK assigns `sessionId` and `_initialized` in exactly one place: while + * handling an `initialize` POST (webStandardStreamableHttp.js:419-420, SDK + * 1.26.0). A non-initialize message on a fresh transport is rejected by + * `validateSession` with "Bad Request: Server not initialized". Replaying those + * two assignments is far less fragile than synthesising an `initialize` request + * through hono's request listener. + * + * This reaches into SDK internals, so it fails LOUDLY rather than silently: if + * the shape moves in an SDK upgrade, `session-adopt.test.ts` goes red at + * `pnpm test` instead of in production. + */ +import type { StreamableHTTPServerTransport } from '@modelcontextprotocol/sdk/server/streamableHttp.js'; + +interface WebStandardInternals { + sessionId?: string | undefined; + _initialized?: boolean; +} + +export function adoptSession(transport: StreamableHTTPServerTransport, sessionId: string): void { + const inner = (transport as unknown as { _webStandardTransport?: WebStandardInternals }) + ._webStandardTransport; + + // Guard on `_initialized` only: the SDK sets it in the constructor, whereas + // `sessionId` is not assigned until an initialize arrives — so on the fresh + // transport we are adopting onto, the property genuinely does not exist yet. + if (!inner || !('_initialized' in inner)) { + throw new Error( + 'MCP SDK internals changed: cannot adopt an existing session id ' + + '(_webStandardTransport._initialized / .sessionId not found). ' + + 'See src/mcplocal/src/http/session-adopt.ts and its test.', + ); + } + + inner.sessionId = sessionId; + inner._initialized = true; +} diff --git a/src/mcplocal/tests/project-mcp-endpoint.test.ts b/src/mcplocal/tests/project-mcp-endpoint.test.ts index 04b5892..d5540b4 100644 --- a/src/mcplocal/tests/project-mcp-endpoint.test.ts +++ b/src/mcplocal/tests/project-mcp-endpoint.test.ts @@ -113,18 +113,26 @@ describe('registerProjectMcpEndpoint', () => { expect(res.json().error).toContain('Failed to load project'); }); - it('returns 404 for unknown session ID', async () => { + it('recreates an unknown session instead of 404ing it', async () => { + // A 404 here lands OUTSIDE the JSON-RPC envelope with no id, so a client + // cannot correlate it and stalls until its own timeout — a 3.5ms failure + // that cost 1800s in production. mcplocal restarts routinely (deploys, RPM + // installs, release.sh), which is exactly when clients hold stale ids. const res = await app.inject({ method: 'POST', url: '/projects/smart-home/mcp', payload: { jsonrpc: '2.0', id: 1, method: 'tools/list', params: {} }, headers: { 'content-type': 'application/json', + 'accept': 'application/json, text/event-stream', 'mcp-session-id': 'nonexistent-session', }, }); - expect(res.statusCode).toBe(404); + expect(res.statusCode).not.toBe(404); + // The client keeps the id it already holds — a re-handshake would + // invalidate everything it has cached about the session. + expect(res.headers['mcp-session-id'] ?? 'nonexistent-session').toBe('nonexistent-session'); }); it('returns 400 for GET without session', async () => { diff --git a/src/mcplocal/tests/session-adopt.test.ts b/src/mcplocal/tests/session-adopt.test.ts new file mode 100644 index 0000000..216efdb --- /dev/null +++ b/src/mcplocal/tests/session-adopt.test.ts @@ -0,0 +1,40 @@ +import { describe, it, expect } from 'vitest'; +import { randomUUID } from 'node:crypto'; +import { StreamableHTTPServerTransport } from '@modelcontextprotocol/sdk/server/streamableHttp.js'; +import { adoptSession } from '../src/http/session-adopt.js'; + +/** + * Canary for SDK upgrades. adoptSession() reaches into + * _webStandardTransport.{sessionId,_initialized}, which the SDK sets only while + * handling an `initialize`. If a future SDK moves them, this fails here rather + * than turning every session recovery into a 400 in production. + */ +describe('adoptSession', () => { + it('marks a fresh transport as initialized with the given session id', () => { + const transport = new StreamableHTTPServerTransport({ sessionIdGenerator: () => randomUUID() }); + expect(transport.sessionId).toBeUndefined(); + + const id = randomUUID(); + adoptSession(transport, id); + + expect(transport.sessionId).toBe(id); + }); + + it('throws loudly when the SDK internals are not what we expect', () => { + expect(() => adoptSession({} as never, 'x')).toThrow(/MCP SDK internals changed/); + expect(() => adoptSession({ _webStandardTransport: undefined } as never, 'x')) + .toThrow(/MCP SDK internals changed/); + }); + + it('satisfies the SDK session check that rejects uninitialized transports', () => { + const transport = new StreamableHTTPServerTransport({ sessionIdGenerator: () => randomUUID() }); + const id = randomUUID(); + adoptSession(transport, id); + + // validateSession() rejects on !_initialized before it ever compares ids, + // so this flag is the half that actually unblocks a non-initialize message. + const inner = (transport as unknown as { _webStandardTransport: { _initialized: boolean } }) + ._webStandardTransport; + expect(inner._initialized).toBe(true); + }); +});