diff --git a/docs/architecture/INDEX.md b/docs/architecture/INDEX.md index f70d151..1882cbf 100644 --- a/docs/architecture/INDEX.md +++ b/docs/architecture/INDEX.md @@ -26,6 +26,7 @@ _Server architecture, transport, connection handling, plugin lifecycle_ | [ADR-103](./core/ADR-103-tls-certificate-and-key-storage-strategy-for-localhost-https.md) | TLS certificate and key storage strategy for localhost HTTPS | Accepted | | [ADR-104](./core/ADR-104-offload-cpu-bound-semantic-operations-to-worker-threads-and-deconflict-the-sse-route.md) | Offload CPU-bound semantic operations to worker threads and deconflict the SSE route | Accepted | | [ADR-105](./core/ADR-105-remove-dormant-worker-offload-path-partial-reversal-of-adr-104.md) | Remove dormant worker-offload path (partial reversal of ADR-104) | Accepted | +| [ADR-106](./core/ADR-106-client-driven-session-re-initialization-via-spec-compliant-http-404.md) | Client-driven session re-initialization via spec-compliant HTTP 404 | Accepted | ## Tools & API _MCP tool design, semantic operations, graph operations, formatters_ diff --git a/docs/architecture/core/ADR-106-client-driven-session-re-initialization-via-spec-compliant-http-404.md b/docs/architecture/core/ADR-106-client-driven-session-re-initialization-via-spec-compliant-http-404.md new file mode 100644 index 0000000..136a356 --- /dev/null +++ b/docs/architecture/core/ADR-106-client-driven-session-re-initialization-via-spec-compliant-http-404.md @@ -0,0 +1,148 @@ +--- +status: Accepted +date: 2026-05-18 +deciders: + - aaronsb + - claude +related: [] +--- + +# ADR-106: Client-driven session re-initialization via spec-compliant HTTP 404 + +## Context + +After ~3h idle, the MCP session is silently dropped and **every** subsequent +tool call fails with HTTP 400 `"Bad Request: Server not initialized"`, +unrecoverable without restarting the MCP client (#128, user-facing symptom; +#190, the engineering investigation). The root cause was proven against +`@modelcontextprotocol/sdk@1.29.0` (the version on `main`) and re-verified +during this work: + +When a request arrived bearing an `Mcp-Session-Id` the server no longer held +a transport for (session evicted), the handler tried a **server-side +synthetic `initialize`**: it built `compatReq = { ...req, headers }` — a +plain object, not a `stream.Readable` — and fed it to SDK 1.29's +`StreamableHTTPServerTransport.handleRequest`. SDK 1.29 delegates to +`@hono/node-server`'s `getRequestListener`, whose `Readable.toWeb(incoming)` +cannot consume a non-stream; even with a real `IncomingMessage`, +`getRequestListener` throws `RequestError("Missing host header")` → 400, and +an `accept` header set post-construction does not survive hono's lazy Web +`Request` reconstruction → SDK 406. **No synthetic-Node-object path reaches +`_initialized = true` on SDK 1.29.** The synthetic init always failed, +"failed open," and fell through to the 400. Two community PRs (#147 real +`IncomingMessage`; #150 retry-with-backoff) were reviewed and declined: the +failure is *deterministic*, not transient, and operates at the wrong layer. + +The Streamable HTTP transport spec already defines the correct recovery +mechanism (Session Management): + +- **§3** — the server **MAY** terminate a session at any time, after which + it **MUST** respond to requests carrying that session ID with **HTTP 404**. +- **§4** — a client receiving **HTTP 404** for a request bearing an + `Mcp-Session-Id` **MUST** start a new session by sending a fresh + `InitializeRequest` with no session ID. +- **§2** — a non-initialize request with no `Mcp-Session-Id` **SHOULD** get + **HTTP 400**. + +The server was doing none of this for the evicted-session case; it +fabricated a phantom transport and returned a non-spec +`400 -32000 "Server not initialized"`, which no client treats as a +session-expiry signal — hence the unrecoverable loop. + +## Decision + +**Stop attempting server-side synthetic initialization. Emit the spec's +session-lifecycle signal and let the client re-initialize itself +(client-driven re-init).** + +In `MCPHttpServer.handleMCPRequest`: + +- Keep the three transport-binding paths unchanged: existing live session; + recreate-on-`initialize` for a known session ID; fresh `initialize` with + no session ID. +- For a non-`initialize` request whose `Mcp-Session-Id` has no live + transport (evicted/stale): respond **HTTP 404** with the + `Mcp-Session-Id` echoed and a courtesy JSON-RPC error body + (`code: -32001`, message instructing re-initialize). Spec §3. +- For a non-`initialize` request with no `Mcp-Session-Id`: respond + **HTTP 400**. Spec §2. +- Delete the synthetic compat-initialize block, the `requireInitializeNotice` + machinery, the `createNullRes`/`NullResponse` shim, and the phantom + transport creation. A spec-compliant client/bridge re-initializes on the + 404 and the **next** request opens a fresh session normally — no client + restart (fixes #128). + +The HTTP status is the load-bearing signal; the JSON-RPC body is courtesy. +`DELETE /mcp` (spec §5) is already handled and is left as-is. + +### Acceptance (restated for this direction) + +#190's original acceptance ("non-initialize request returns 2xx") was +written before a direction was chosen and described the *server-heal* +candidate. For client-driven re-init the equivalent recovery is: + +> A non-`initialize` request bearing an evicted `Mcp-Session-Id` returns +> **HTTP 404 with the `Mcp-Session-Id` header set** (spec §3 signal), **and** +> a subsequent `initialize` request with no session ID returns **2xx with a +> fresh `Mcp-Session-Id`** (a working new session). Together the client +> recovers without a restart, per spec §4. Asserted by a harness driving the +> request pair through the handler against the SDK version on `main`. + +### Known client caveat + +Spec §4 is the client's obligation. `mcp-remote` does not "clear session + +reinit" explicitly; on a 404 it recurses through `connectToRemoteServer` +with a fresh transport, whose first message is an `initialize` with no +session ID — the same observable outcome. Some clients (e.g. certain Claude +Code builds) reportedly do not honor §4 at all. That is an upstream client +gap, not something the server can paper over without re-introducing the +proven-broken synthetic-init class. The server's contract is the spec; +end-to-end recovery is validated by functional test against the real +client/bridge. + +## Consequences + +### Positive + +- #128 becomes recoverable without a client restart on spec-compliant + clients/bridges; the unrecoverable 400 loop is gone. +- Deletes a large block of proven-broken, SDK-version-fragile synthetic-init + code and its shims; the handler now expresses exactly the spec's state + machine. +- No dependency on SDK internals or hono request reconstruction — robust + across SDK upgrades. + +### Negative + +- Recovery depends on the client honoring spec §4. Clients that don't will + still need a manual reconnect — but they did before too, and the server is + now spec-correct rather than emitting a misleading non-spec 400. + Mitigation: validated by functional test against the actual + Claude/`mcp-remote` path. + +### Neutral + +- Requires a functional re-test against a real client (BRAT prerelease → + reconnect) to confirm end-to-end recovery; covered by the project's + prerelease loop. +- A deterministic regression harness drives the synthetic request pair + through `handleMCPRequest` and asserts the 404+header / 2xx-reinit pair; + added with this change. + +## Alternatives Considered + +- **#147 — real synthetic `IncomingMessage` round-tripped through hono.** + Rejected: still cannot reach `_initialized` on SDK 1.29 (RequestError → + 400, then 406); right instinct, wrong layer (proven empirically). +- **#150 — retry the synthetic init with backoff.** Rejected: the failure + is deterministic, not transient; retry only adds latency before the + identical failure. +- **Cosmetic status rewrite (`-32001 "session expired"`).** Rejected: a + body relabel is not a recovery; clients still don't re-init and the loop + persists. Explicitly called out as a non-fix in #190. +- **Server-side session keep-alive / TTL extension to prevent the drop.** + Rejected as the primary fix: it narrows the window but does not make a + *dropped* session recoverable (network loss, restart, eviction under + pressure still strand the client). Orthogonal; may be revisited + separately. The spec's 404/re-init path is the designed recovery and is + needed regardless. diff --git a/src/mcp-server.ts b/src/mcp-server.ts index 857c1d6..ae0dd7d 100644 --- a/src/mcp-server.ts +++ b/src/mcp-server.ts @@ -58,22 +58,6 @@ interface ServerWithTimeouts { setTimeout: (msecs: number) => unknown; } -/** Null response shim for internal initialize calls */ -interface NullResponse { - writeHead: (status: number, headers?: Record) => void; - setHeader: (k: string, v: string) => void; - getHeader: (k: string) => string | undefined; - statusCode: number; - headersSent: boolean; - write: (chunk: unknown) => void; - end: (data?: unknown) => void; - on: (event: string, handler: (...args: unknown[]) => void) => void; - once: (event: string, handler: (...args: unknown[]) => void) => void; - status: (v: number) => NullResponse; - json: (body: unknown) => void; - send: (body?: unknown) => void; -} - /** Connection pool stats response */ interface ConnectionPoolStatsResponse { enabled: boolean; @@ -104,7 +88,10 @@ export class MCPHttpServer { private connectionCount: number = 0; private plugin?: MCPPluginRef; // Reference to the plugin private connectionPool?: ConnectionPool; - private sessionManager?: SessionManager; + // Assigned unconditionally in the constructor. Non-optional so the + // "initialize is never short-circuited as a terminated session" invariant + // (ADR-106) is guaranteed by the type system, not just construction order. + private sessionManager: SessionManager; private certificateManager: CertificateManager | null; private isHttps: boolean = false; @@ -380,6 +367,57 @@ export class MCPHttpServer { }); } + /** + * Emit the Streamable HTTP spec's session-lifecycle signal so the client + * can recover a dropped/evicted session on its own (client-driven re-init, + * ADR-106). + * + * - If the request carried an `Mcp-Session-Id` we no longer hold a + * transport for, the session is terminated: respond **HTTP 404** + * (Session Management §3). A spec-compliant client/bridge MUST then + * start a new session by sending a fresh `InitializeRequest` with no + * session ID (§4) — no client restart required, fixing #128. + * - If no `Mcp-Session-Id` was sent on a non-initialize request, a session + * is required: respond **HTTP 400** (§2). + * + * The HTTP status is the load-bearing signal; the JSON-RPC error body is + * courtesy for clients that surface it. We deliberately do not attempt a + * server-side synthetic initialize — that cannot drive SDK 1.29's + * web-standard transport to an initialized state (see #190). + */ + private sendSessionTerminated( + res: express.Response, + request: JsonRpcRequest | undefined, + sessionId: string | undefined + ): void { + const id = request?.id ?? null; + if (sessionId) { + // Spec §3: terminated session → 404; client re-inits per §4. + res.setHeader('Mcp-Session-Id', sessionId); + res.status(404).json({ + jsonrpc: '2.0', + error: { + code: -32001, + message: 'Session expired or not found. Start a new session by sending an initialize request without a session ID.', + data: { sessionId } + }, + id + }); + Debug.log(`🔁 Session ${sessionId} terminated → 404 (client should re-initialize per MCP spec §4)`); + return; + } + // Spec §2: session required for non-initialize requests → 400. + res.status(400).json({ + jsonrpc: '2.0', + error: { + code: -32600, + message: 'Bad Request: a session is required. Send an initialize request first.' + }, + id + }); + Debug.log('⚠️ Non-initialize request with no session id → 400 (session required)'); + } + private async handleMCPRequest(req: express.Request, res: express.Response): Promise { try { const request = req.body as JsonRpcRequest | undefined; @@ -405,35 +443,6 @@ export class MCPHttpServer { } let mcpServer: MCPServer; - // Helper: create a null response shim so we can send an internal initialize - const createNullRes = (): NullResponse => { - const headers: Record = {}; - let sent = false; - let code = 200; - const resp: NullResponse = { - // Node-style response interface (minimal no-op implementation) - writeHead: (_status: number, _headers?: Record) => { code = _status; sent = true; }, - setHeader: (k: string, v: string) => { headers[k] = v; }, - getHeader: (k: string) => headers[k], - get statusCode() { return code; }, - set statusCode(v: number) { code = v; }, - get headersSent() { return sent; }, - write: (_chunk: unknown) => { /* no-op */ }, - end: (_data?: unknown) => { sent = true; }, - on: (_event: string, _handler: (...args: unknown[]) => void) => { /* no-op */ }, - once: (_event: string, _handler: (...args: unknown[]) => void) => { /* no-op */ }, - // Express-like helpers (used by some wrappers) - status: (v: number) => { code = v; return resp; }, - json: (_body: unknown) => { sent = true; }, - send: (_body?: unknown) => { sent = true; }, - }; - return resp; - }; - // When a non-initialize request arrives without an active transport, - // we return a JSON-RPC error instructing the client to initialize using - // the provided session ID. This avoids fragile internal auto-initialize. - let requireInitializeNotice = false; - // Helper: register transport with lifecycle hooks const attachTransportHandlers = (sessId: string, tr: StreamableHTTPServerTransport) => { try { @@ -488,18 +497,18 @@ export class MCPHttpServer { this.connectionCount++; Debug.log(`♻️ Recreated transport for session ${sessionId} (requests: ${session.requestCount})`); } else { - // Create transport and require client initialize on next call - const newSessionId = sessionId; // reuse provided id (guaranteed by branch) - mcpServer = this.mcpServerPool.getOrCreateServer(newSessionId); - effectiveSessionId = newSessionId; - transport = new StreamableHTTPServerTransport({ - sessionIdGenerator: () => effectiveSessionId - }); - await mcpServer.connect(transport); - this.transports.set(effectiveSessionId, transport); - attachTransportHandlers(effectiveSessionId, transport); - this.connectionCount++; - requireInitializeNotice = true; + // Stale/evicted session: the client presented an Mcp-Session-Id + // we no longer hold a transport for, and this is not an + // initialize request. Per the Streamable HTTP spec (Session + // Management §3) the server MUST respond HTTP 404 for a + // terminated session; per §4 the client must then start a new + // session by sending a fresh InitializeRequest with no session + // ID. We do NOT fabricate a transport or attempt a server-side + // synthetic initialize — that cannot drive SDK 1.29's + // web-standard transport to an initialized state (ADR-106 / + // #190) and only produces an unrecoverable 400 loop (#128). + this.sendSessionTerminated(res, request, sessionId); + return; } } else if (!sessionId && isInitializeRequest(request)) { // New initialization request - create new transport with session @@ -525,91 +534,25 @@ export class MCPHttpServer { this.sessionManager.getOrCreateSession(effectiveSessionId); } } else { - // No or unknown session on non-initialize request. - // Generate a session (or reuse provided) and require client initialize next. - const newSessionId = sessionId ?? randomUUID(); - mcpServer = this.mcpServerPool.getOrCreateServer(newSessionId); - effectiveSessionId = newSessionId; - transport = new StreamableHTTPServerTransport({ - sessionIdGenerator: () => effectiveSessionId - }); - await mcpServer.connect(transport); - this.transports.set(effectiveSessionId, transport); - attachTransportHandlers(effectiveSessionId, transport); - this.connectionCount++; - requireInitializeNotice = true; + // Non-initialize request with no usable session. Either: + // - an Mcp-Session-Id we don't hold a transport for → spec §3 + // terminated-session signal (HTTP 404), client re-inits per §4; + // - no Mcp-Session-Id at all and not an initialize → spec §2 + // "session required" (HTTP 400). + // sendSessionTerminated picks the status from sessionId presence. + // No phantom transport, no synthetic initialize (see #190/#128). + this.sendSessionTerminated(res, request, sessionId); + return; } - // Compatibility: if we just created a transport for a non-initialize call, - // attempt a safe, internal initialize to avoid client retry loops. - if (requireInitializeNotice && transport && !isInitializeRequest(request)) { - // Clone req to ensure session header is present for compat initialize - const compatReq = { - ...req, - headers: { - ...req.headers, - 'mcp-session-id': effectiveSessionId - } - } as unknown as IncomingMessage; - const versionsToTry = ['2025-06-18', '2024-11-05', '1.0']; - let initOk = false; - for (const ver of versionsToTry) { - try { - const initReq = { - jsonrpc: '2.0', - id: '__compat_init__', - method: 'initialize', - params: { - protocolVersion: ver, - capabilities: {}, - clientInfo: { name: 'obsidian-mcp-compat', version: getVersion() } - } - } as unknown; - await transport.handleRequest(compatReq, createNullRes() as unknown as ServerResponse, initReq); - initOk = true; - Debug.log(`Compat initialize succeeded with protocolVersion=${ver}`); - break; - } catch (e) { - Debug.error(`Compat initialize attempt failed (protocolVersion=${ver}):`, e); - } - } - // Fail-open: even if initialize failed, proceed with the original request - // to avoid client retry loops. The transport/server may still reject it, - // but this gives us a concrete server error to act on. - requireInitializeNotice = false; - if (!initOk) { - Debug.log('Compat initialize failed; proceeding without explicit initialize (fail-open).'); - } - } - - // If initialization is still required and this isn't an initialize request, - // instruct the client to initialize for this session. - if (requireInitializeNotice && !isInitializeRequest(request)) { - const id = request?.id ?? null; - res.setHeader('Mcp-Session-Id', effectiveSessionId); - res.status(400).json({ - jsonrpc: '2.0', - error: { - code: -32000, - message: 'Bad Request: Server not initialized', - data: { sessionId: effectiveSessionId } - }, - id - }); - return; - } - - // Safety: ensure we have a transport before forwarding + // Safety: every reachable path above either bound a live `transport` + // (existing session, recreate-on-initialize, fresh initialize) or + // returned a spec-compliant terminated/required-session response. A + // missing transport here is an unexpected invariant break, not a stale + // session — surface it explicitly rather than papering it. if (!transport) { - const id = request?.id ?? null; - if (effectiveSessionId) { - res.setHeader('Mcp-Session-Id', effectiveSessionId); - } - res.status(200).json({ - jsonrpc: '2.0', - error: { code: -32001, message: 'No active transport/session. Please initialize and retry.', data: effectiveSessionId ? { sessionId: effectiveSessionId } : undefined }, - id - }); + Debug.error('Invariant: no transport after session resolution'); + this.sendSessionTerminated(res, request, sessionId); return; } diff --git a/tests/mcp-session-reinit.test.ts b/tests/mcp-session-reinit.test.ts new file mode 100644 index 0000000..60e8432 --- /dev/null +++ b/tests/mcp-session-reinit.test.ts @@ -0,0 +1,134 @@ +/** + * Regression harness for #190 / #128 (ADR-106): client-driven session + * re-initialization via the spec-compliant HTTP 404 signal. + * + * The pre-ADR-106 server tried a server-side synthetic `initialize` for an + * evicted session, which cannot drive SDK 1.29's web-standard transport to an + * initialized state, "failed open," and returned a non-spec + * `400 -32000 "Server not initialized"` that no client treats as a + * session-expiry signal — an unrecoverable loop until client restart. + * + * ADR-106: a non-`initialize` request bearing an evicted `Mcp-Session-Id` + * must return HTTP 404 with the `Mcp-Session-Id` echoed (spec §3) so a + * compliant client/bridge re-initializes per §4; a non-`initialize` request + * with no session id must return HTTP 400 (spec §2); an `initialize` request + * must never be short-circuited as "terminated". + * + * These paths return before any SDK transport is constructed, so they are + * deterministic without exercising the SDK transport in jsdom (the exact + * thing #190 established cannot be driven synthetically). + */ +import { MCPHttpServer } from '../src/mcp-server'; +import { App } from 'obsidian'; + +jest.mock('fs', () => ({ + existsSync: jest.fn(() => false), + mkdirSync: jest.fn(), + readFileSync: jest.fn(), + writeFileSync: jest.fn() +})); + +interface MockRes { + statusCode: number; + headers: Record; + body: unknown; + headersSent: boolean; + setHeader: (k: string, v: string) => void; + status: (c: number) => MockRes; + json: (b: unknown) => void; +} + +function makeRes(): MockRes { + const res: MockRes = { + statusCode: 0, + headers: {}, + body: undefined, + headersSent: false, + setHeader(k, v) { this.headers[k.toLowerCase()] = v; }, + status(c) { this.statusCode = c; return this; }, + json(b) { this.body = b; this.headersSent = true; } + }; + return res; +} + +function makeReq(body: unknown, sessionId?: string) { + return { + body, + headers: sessionId ? { 'mcp-session-id': sessionId } : {} + }; +} + +describe('MCP session re-initialization signal (ADR-106 / #190 / #128)', () => { + let server: MCPHttpServer; + + beforeEach(() => { + const mockApp = new App(); + mockApp.vault = { + ...mockApp.vault, + adapter: { basePath: '/mock/vault/path' } + } as any; + server = new MCPHttpServer(mockApp, 3001); + }); + + test('evicted session + non-initialize request → HTTP 404 with Mcp-Session-Id echoed (spec §3)', async () => { + const res = makeRes(); + const staleSession = 'stale-session-id-1234'; + // No transport is registered for this id → evicted/terminated. + await (server as any).handleMCPRequest( + makeReq({ jsonrpc: '2.0', id: 7, method: 'tools/call', params: {} }, staleSession), + res + ); + + expect(res.statusCode).toBe(404); + expect(res.headers['mcp-session-id']).toBe(staleSession); + // Must NOT be the old unrecoverable non-spec signal. + expect(res.statusCode).not.toBe(400); + const body = res.body as { error?: { code?: number; message?: string } }; + expect(body.error?.code).toBe(-32001); + expect(body.error?.message).toMatch(/initialize/i); + }); + + test('non-initialize request with no session id → HTTP 400 (spec §2)', async () => { + const res = makeRes(); + await (server as any).handleMCPRequest( + makeReq({ jsonrpc: '2.0', id: 8, method: 'tools/call', params: {} }), + res + ); + + expect(res.statusCode).toBe(400); + expect(res.headers['mcp-session-id']).toBeUndefined(); + }); + + test('initialize request is never short-circuited as a terminated session', async () => { + const spy = jest.spyOn(server as any, 'sendSessionTerminated'); + + // (a) initialize with no session id (fresh) and (b) initialize bearing a + // stale session id (recreate path) must both bypass the 404/400 signal. + for (const sid of [undefined, 'previously-evicted-id']) { + const res = makeRes(); + await (server as any).handleMCPRequest( + makeReq( + { jsonrpc: '2.0', id: 1, method: 'initialize', params: { protocolVersion: '2025-06-18', capabilities: {}, clientInfo: { name: 't', version: '0' } } }, + sid + ), + res + ); + expect(res.statusCode).not.toBe(404); + } + expect(spy).not.toHaveBeenCalled(); + spy.mockRestore(); + }); + + test('sendSessionTerminated emits the spec signals directly', () => { + const withId = makeRes(); + (server as any).sendSessionTerminated(withId, { id: 3 }, 'sess-abc'); + expect(withId.statusCode).toBe(404); + expect(withId.headers['mcp-session-id']).toBe('sess-abc'); + expect((withId.body as { id?: unknown }).id).toBe(3); + + const noId = makeRes(); + (server as any).sendSessionTerminated(noId, { id: null }, undefined); + expect(noId.statusCode).toBe(400); + expect(noId.headers['mcp-session-id']).toBeUndefined(); + }); +});