fix(mcplocal): bounded, legible MCP failures — no request can hang forever #127

Merged
michal merged 9 commits from fix/bounded-mcp-failures into main 2026-08-25 23:03:51 +00:00
5 changed files with 179 additions and 11 deletions
Showing only changes of commit 0eca4148e3 - Show all commits

View File

@@ -30,6 +30,8 @@ import { AuditCollector } from '../audit/collector.js';
import { WireNameCodec, routeWithWireNames } from '../util/wire-names.js'; import { WireNameCodec, routeWithWireNames } from '../util/wire-names.js';
import { newTraceCode } from '../util/trace-code.js'; import { newTraceCode } from '../util/trace-code.js';
import { runInRequestScope } from '../request-context.js'; import { runInRequestScope } from '../request-context.js';
import { adoptSession } from './session-adopt.js';
import { degradationNotice } from '../util/degrade.js';
interface ProjectCacheEntry { interface ProjectCacheEntry {
router: McpRouter; router: McpRouter;
@@ -48,6 +50,12 @@ export function registerProjectMcpEndpoint(app: FastifyInstance, mcpdClient: Mcp
let resolvedUserName: string | null | undefined; // undefined = not yet resolved let resolvedUserName: string | null | undefined; // undefined = not yet resolved
const projectCache = new Map<string, ProjectCacheEntry>(); const projectCache = new Map<string, ProjectCacheEntry>();
const sessions = new Map<string, SessionEntry>(); const sessions = new Map<string, SessionEntry>();
/**
* 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<string>();
// Wire-name codecs are keyed per project and OUTLIVE the router cache TTL: // 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, // a client may call a tool it listed minutes ago through a refreshed router,
// and the mapping must still resolve. // and the mapping must still resolve.
@@ -217,6 +225,27 @@ export function registerProjectMcpEndpoint(app: FastifyInstance, mcpdClient: Mcp
} }
// POST /projects/:projectName/mcp — JSON-RPC requests // 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) => { app.post<{ Params: { projectName: string } }>('/projects/:projectName/mcp', async (request, reply) => {
const { projectName } = request.params; const { projectName } = request.params;
const sessionId = request.headers['mcp-session-id'] as string | undefined; const sessionId = request.headers['mcp-session-id'] as string | undefined;
@@ -229,10 +258,15 @@ export function registerProjectMcpEndpoint(app: FastifyInstance, mcpdClient: Mcp
return; return;
} }
if (sessionId && !sessions.has(sessionId)) { // An unknown session id means mcplocal restarted (or the session was
reply.code(404).send({ error: 'Session not found' }); // evicted) while the client still holds a perfectly good id. Replying 404
return; // 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 // New session — get/create project router
let router: McpRouter; let router: McpRouter;
@@ -243,9 +277,11 @@ export function registerProjectMcpEndpoint(app: FastifyInstance, mcpdClient: Mcp
return; return;
} }
const transport = new StreamableHTTPServerTransport({ // Called by onsessioninitialized on the normal path, and explicitly on the
sessionIdGenerator: () => randomUUID(), // recreation path — where onsessioninitialized never fires. Missing any of
onsessioninitialized: (id) => { // 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 }); sessions.set(id, { transport, projectName });
trafficCapture?.emit({ trafficCapture?.emit({
timestamp: new Date().toISOString(), timestamp: new Date().toISOString(),
@@ -283,7 +319,11 @@ export function registerProjectMcpEndpoint(app: FastifyInstance, mcpdClient: Mcp
verified: true, verified: true,
payload: { projectName }, payload: { projectName },
}); });
}, };
const transport = new StreamableHTTPServerTransport({
sessionIdGenerator: () => randomUUID(),
onsessioninitialized: (id) => { registerSession(id); },
}); });
// Per-request correlationId map for linking client ↔ upstream event pairs. // Per-request correlationId map for linking client ↔ upstream event pairs.
@@ -395,7 +435,9 @@ export function registerProjectMcpEndpoint(app: FastifyInstance, mcpdClient: Mcp
}); });
requestCorrelations.delete(requestId); 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); await transport.handleRequest(request.raw, reply.raw, request.body);
reply.hijack(); reply.hijack();
}); });

View File

@@ -42,6 +42,24 @@ export async function createHttpServer(
methods: ['GET', 'POST', 'PUT', 'DELETE', 'PATCH'], 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 // Health endpoint
app.get('/health', async (_request, reply) => { app.get('/health', async (_request, reply) => {
const upstreams = deps.router.getUpstreamNames(); const upstreams = deps.router.getUpstreamNames();

View File

@@ -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;
}

View File

@@ -113,18 +113,26 @@ describe('registerProjectMcpEndpoint', () => {
expect(res.json().error).toContain('Failed to load project'); 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({ const res = await app.inject({
method: 'POST', method: 'POST',
url: '/projects/smart-home/mcp', url: '/projects/smart-home/mcp',
payload: { jsonrpc: '2.0', id: 1, method: 'tools/list', params: {} }, payload: { jsonrpc: '2.0', id: 1, method: 'tools/list', params: {} },
headers: { headers: {
'content-type': 'application/json', 'content-type': 'application/json',
'accept': 'application/json, text/event-stream',
'mcp-session-id': 'nonexistent-session', '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 () => { it('returns 400 for GET without session', async () => {

View File

@@ -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);
});
});