diff --git a/mcpb/server.js b/mcpb/server.js index 04a3470..18baf5f 100644 --- a/mcpb/server.js +++ b/mcpb/server.js @@ -122,13 +122,23 @@ async function reinitialize() { } sessionId = sid; - // Best-effort: a stateful server expects `notifications/initialized` before - // it will service tool calls. It's a notification (202, no body to emit). + // Complete the handshake. If the server gates tool calls on + // `notifications/initialized` this is load-bearing; if not, it's harmless. + // Either way it's best-effort — a lost notification just means the replay + // 404s again and recovery terminates cleanly via the single-attempt cap in + // dispatch. Log a non-2xx (or a throw) so a required-but-failing + // notification is diagnosable from stderr instead of silently swallowed. try { const note = await postOnce({ jsonrpc: '2.0', method: 'notifications/initialized' }); + if (note.status !== 200 && note.status !== 202) { + process.stderr.write(`[obsidian-mcp-bridge] reinit: notifications/initialized → HTTP ${note.status}\n`); + } await note.body?.cancel().catch(() => {}); - } catch { /* server may not require it; the replay will reveal the truth */ } + } catch (e) { + process.stderr.write(`[obsidian-mcp-bridge] reinit: notifications/initialized failed: ${e.message}\n`); + } + // Fire-and-forget; startNotifyStream self-handles its own errors internally. startNotifyStream(); return true; } @@ -161,13 +171,21 @@ async function dispatch(message, isReplay = false) { } } - if (response.status === 404 && sessionId) { - // The server evicted/lost our session (idle GC, or a server restart). Per - // MCP spec the 404 means "re-initialize," but some clients (e.g. Claude - // Desktop) don't act on it and the connection dead-ends. Self-heal once: - // re-establish a session and replay this exact message so recovery is - // invisible to the client (#238). `isReplay` caps it at a single attempt - // so a session that dies again instantly can't spin a reinit loop. + if (response.status === 404) { + // A 404 on /mcp means the server terminated/lost our session (idle GC or a + // server restart); per MCP spec the bridge should re-initialize, but some + // clients (e.g. Claude Desktop) don't act on the signal and the connection + // dead-ends. We deliberately do NOT gate on the bridge's current + // `sessionId`: a concurrent sibling's reinit may have already nulled it, + // and the 404 itself proves a session id was presented on this request + // (the server answers 400, handled below, when none is). So any 404 on a + // non-initialize call is a self-heal trigger. + // + // Self-heal once: re-establish a session and replay this exact message so + // recovery is invisible to the client (#238). Concurrent 404s coalesce + // onto a single reinit via the `initializing` gate and each replays under + // the fresh session. `isReplay` caps it at one attempt, so a session that + // dies again instantly terminates with -32000 rather than looping. if (!isReplay && message.method !== 'initialize' && cachedInitialize) { if (!initializing) { initializing = reinitialize().finally(() => { initializing = null; }); diff --git a/tests/bridge-self-heal.test.ts b/tests/bridge-self-heal.test.ts index cc194fc..255ea39 100644 --- a/tests/bridge-self-heal.test.ts +++ b/tests/bridge-self-heal.test.ts @@ -21,7 +21,7 @@ process.env.MCP_API_KEY = 'test-key'; const bridge = require('../mcpb/server.js') as { dispatch: (msg: unknown, isReplay?: boolean) => Promise; __reset: () => void; - __state: () => { sessionId: string | null; cachedInitialize: unknown }; + __state: () => { sessionId: string | null; cachedInitialize: unknown; initializing: unknown }; }; interface FakeResponse { @@ -146,6 +146,36 @@ describe('mcpb bridge self-heal on session expiry (#238)', () => { expect(calls.filter(c => c.rpc === 'tools/call')).toHaveLength(1); }); + test('concurrent 404s coalesce onto a single reinit; each call replays under the fresh session', async () => { + let sidSeq = 0; + installFetch((rec) => { + if (rec.rpc === 'initialize') { + sidSeq += 1; + return res(200, { sid: `sess-${sidSeq}`, json: { jsonrpc: '2.0', id: rec.id, result: {} } }); + } + if (rec.rpc === 'notifications/initialized') return res(202); + if (rec.session === 'sess-1') return res(404, { sid: 'sess-1', json: { jsonrpc: '2.0', id: rec.id, error: { code: -32001, message: 'expired' } } }); + return res(200, { json: { jsonrpc: '2.0', id: rec.id, result: { ok: true } } }); + }); + + await bridge.dispatch(INIT); + // Two tool calls in flight together: both 404 on sess-1 and must coalesce + // onto one re-init rather than each kicking off its own handshake. + const A = { jsonrpc: '2.0', id: 1, method: 'tools/call', params: { name: 'vault', arguments: { action: 'list' } } }; + const B = { jsonrpc: '2.0', id: 2, method: 'tools/call', params: { name: 'vault', arguments: { action: 'read' } } }; + await Promise.all([bridge.dispatch(A), bridge.dispatch(B)]); + + // Both clients get genuine results, neither sees the expiry error. + expect((emitted.find(m => m.id === 1) as { result?: { ok?: boolean } } | undefined)?.result?.ok).toBe(true); + expect((emitted.find(m => m.id === 2) as { result?: { ok?: boolean } } | undefined)?.result?.ok).toBe(true); + expect(emitted.some(m => (m as { error?: { code?: number } }).error?.code === -32000)).toBe(false); + // Exactly one self-heal despite two concurrent 404s: 1 initial + 1 reinit. + expect(calls.filter(c => c.rpc === 'initialize')).toHaveLength(2); + // Each tool call was attempted on the dead session and replayed on the new. + expect(calls.filter(c => c.rpc === 'tools/call' && c.session === 'sess-1')).toHaveLength(2); + expect(calls.filter(c => c.rpc === 'tools/call' && c.session === 'sess-2')).toHaveLength(2); + }); + test('initialize handshake is cached so a later session loss can be replayed', async () => { installFetch((rec) => { if (rec.rpc === 'initialize') return res(200, { sid: 'sess-1', json: { jsonrpc: '2.0', id: rec.id, result: {} } });