diff --git a/packages/mcp/src/browser-rpc.ts b/packages/mcp/src/browser-rpc.ts index ce2924743..ed31dedde 100644 --- a/packages/mcp/src/browser-rpc.ts +++ b/packages/mcp/src/browser-rpc.ts @@ -17,6 +17,8 @@ type BrowserRPCBridgeOptions = { onConnectionChange: () => void } +type ConnectionListener = (connected: boolean) => void + type BrowserMessage = { type: string id?: string @@ -67,6 +69,7 @@ export function createBrowserRPCBridge({ authToken, onConnectionChange }: Browse // register message. Unauthenticated clients can only send register; // all other message types (request, response) are rejected. const authenticatedClients = new Set() + const connectionListeners = new Set() let browserWs: WebSocket | null = null let browserRegistered = false let bridgeClosed = false @@ -75,6 +78,18 @@ export function createBrowserRPCBridge({ authToken, onConnectionChange }: Browse return Boolean(browserWs && browserRegistered) } + function notifyConnectionChange() { + onConnectionChange() + const connected = isConnected() + for (const listener of connectionListeners) listener(connected) + } + + function subscribeConnectionChange(listener: ConnectionListener): () => void { + connectionListeners.add(listener) + listener(isConnected()) + return () => connectionListeners.delete(listener) + } + function notifyConnectionWaiters() { for (const waiter of connectionWaiters) { clearTimeout(waiter.timer) @@ -221,7 +236,7 @@ export function createBrowserRPCBridge({ authToken, onConnectionChange }: Browse } } notifyConnectionWaiters() - onConnectionChange() + notifyConnectionChange() broadcastRegisterPrompt() } @@ -306,7 +321,7 @@ export function createBrowserRPCBridge({ authToken, onConnectionChange }: Browse // CLOSING→CLOSED transition), the waiter should keep waiting the full // APP_WAIT_TIMEOUT for a reconnect. registerBrowser will resolve it // via notifyConnectionWaiters if the browser reconnects in time. - onConnectionChange() + notifyConnectionChange() } function handleConnection(ws: WebSocket) { @@ -330,11 +345,13 @@ export function createBrowserRPCBridge({ authToken, onConnectionChange }: Browse browserRegistered = false clients.clear() authenticatedClients.clear() + connectionListeners.clear() } return { close, isConnected, + subscribeConnectionChange, sendRPC, handleConnection, handleMessage, diff --git a/packages/mcp/src/index.ts b/packages/mcp/src/index.ts index 75e4efcc7..df9527176 100644 --- a/packages/mcp/src/index.ts +++ b/packages/mcp/src/index.ts @@ -18,7 +18,12 @@ if (process.argv.includes('--help') || process.argv.includes('-h')) { ` OPENPENCIL_MCP_AUTH_TOKEN Bearer token for MCP and RPC auth\n` + ` OPENPENCIL_MCP_ROOT Allowed directory for file-scoped tools (default: current working directory)\n` + ` OPENPENCIL_MCP_EVAL Set to 1 to enable the eval tool\n` + - ` OPENPENCIL_MCP_CORS_ORIGIN Allowed CORS origin\n` + ` OPENPENCIL_MCP_CORS_ORIGIN Allowed CORS origin\n` + + ` OPENPENCIL_MCP_APP_TIMEOUT_MS If set, close the server and remove its discovery\n` + + ` file after no app is attached for this many ms. The\n` + + ` grace period starts at startup and after disconnects.\n` + + ` Unset/0 disables it (default) — do not set this for\n` + + ` manual/CLI use, since nothing may ever register.\n` ) process.exit(0) } @@ -41,6 +46,25 @@ const port = rawPort // the PORT value alone determines whether TCP is enabled (PORT=0 is the kill switch). const withTcp = port > 0 +const MAX_APP_TIMEOUT_MS = 2_147_483_647 +const rawAppTimeoutText = process.env.OPENPENCIL_MCP_APP_TIMEOUT_MS?.trim() +let appAttachTimeoutMs: number | undefined +if (rawAppTimeoutText) { + if (!/^\d+$/.test(rawAppTimeoutText)) { + process.stderr.write( + `Error: OPENPENCIL_MCP_APP_TIMEOUT_MS must be a non-negative integer, got "${rawAppTimeoutText}"\n` + ) + process.exit(1) + } + appAttachTimeoutMs = Number.parseInt(rawAppTimeoutText, 10) + if (!Number.isSafeInteger(appAttachTimeoutMs) || appAttachTimeoutMs > MAX_APP_TIMEOUT_MS) { + process.stderr.write( + `Error: OPENPENCIL_MCP_APP_TIMEOUT_MS must be an integer in 0–${MAX_APP_TIMEOUT_MS}, got "${rawAppTimeoutText}"\n` + ) + process.exit(1) + } +} + const handle = await startServer({ httpPort: withTcp ? port : 0, withTcp, @@ -64,7 +88,8 @@ const handle = await startServer({ } return trimmed })(), - corsOrigin: process.env.OPENPENCIL_MCP_CORS_ORIGIN?.trim() || null + corsOrigin: process.env.OPENPENCIL_MCP_CORS_ORIGIN?.trim() || null, + appAttachTimeoutMs }) process.stderr.write(`OpenPencil MCP server\n`) diff --git a/packages/mcp/src/server.ts b/packages/mcp/src/server.ts index b3efe91ad..bb9752a5e 100644 --- a/packages/mcp/src/server.ts +++ b/packages/mcp/src/server.ts @@ -70,6 +70,17 @@ export interface ServerOptions { /** Auth token for /mcp and /rpc endpoints. Auto-generated (32-hex) when omitted. Pass null explicitly to disable auth. */ authToken?: string | null corsOrigin?: string | null + /** + * If set, the server starts a grace-period timer while no app is attached. + * The timer closes the server and removes its discovery file unless an app + * registers before it expires. A later app disconnect starts a new grace + * period, which lets app-spawned servers survive brief reloads while still + * cleaning up after renderer or process crashes. Undefined/0 disables the + * watchdog — the default, since a bare CLI invocation for manual testing + * should not self-terminate just because nobody connected yet. The desktop + * app opts in when it spawns the server. + */ + appAttachTimeoutMs?: number } export interface ServerHandle { @@ -391,6 +402,7 @@ function buildHandle( } export async function startServer(options: ServerOptions = {}): Promise { + validateAppAttachTimeout(options.appAttachTimeoutMs) const ctx = buildServerContext(options) // Wire shared connection handling BEFORE starting listeners so that @@ -429,7 +441,7 @@ export async function startServer(options: ServerOptions = {}): Promise MAX_TIMER_MS) { + throw new RangeError(`appAttachTimeoutMs must be in the range 0–${MAX_TIMER_MS}`) + } +} + +/** + * Closes an app-spawned server after it remains unattached for the configured + * grace period. Registering an app cancels the pending shutdown; disconnecting + * starts a fresh grace period so renderer reloads can reconnect without + * leaving a permanently orphaned process behind. + */ +function armAppAttachWatchdog( + timeoutMs: number | undefined, + browserRPC: ReturnType, + handle: ServerHandle +): void { + if (timeoutMs === undefined || timeoutMs === 0) return + + let timer: ReturnType | null = null + let closing = false + const clearTimer = () => { + if (!timer) return + clearTimeout(timer) + timer = null + } + const armTimer = () => { + clearTimer() + timer = setTimeout(() => { + timer = null + if (browserRPC.isConnected() || closing) return + closing = true + unsubscribe() + void handle.close().catch((e) => { + console.error('[MCP] Watchdog: failed to close orphaned (no_app) server:', e) + }) + }, timeoutMs) + timer.unref() + } + const unsubscribe = browserRPC.subscribeConnectionChange((connected) => { + if (connected) clearTimer() + else armTimer() + }) } diff --git a/src/app/automation/mcp/spawn.ts b/src/app/automation/mcp/spawn.ts index fb953b714..c5b78695d 100644 --- a/src/app/automation/mcp/spawn.ts +++ b/src/app/automation/mcp/spawn.ts @@ -30,6 +30,11 @@ const APP_VERSION = const noop = () => undefined const MAX_STARTUP_STDERR_LENGTH = 8_192 const MCP_EXECUTABLE = 'openpencil-mcp-http' +// While no app is attached, the spawned server waits this long for a register +// or reconnect before closing itself and removing its discovery file. This +// prevents a server that outlives a crashed/reloaded app from squatting the +// port forever while still allowing brief renderer reloads (issue #488). +const MCP_APP_ATTACH_TIMEOUT_MS = 30_000 let runtimeAutomationAuthToken: string | null = DEV_AUTOMATION_AUTH_TOKEN let runtimeAutomationStartupError: Error | null = null @@ -279,7 +284,8 @@ async function startMCPIfNeeded(): Promise { OPENPENCIL_MCP_AUTH_TOKEN: authToken, OPENPENCIL_MCP_CORS_ORIGIN: window.location.origin, OPENPENCIL_MCP_TCP: '1', - OPENPENCIL_MCP_ROOT: mcpRoot + OPENPENCIL_MCP_ROOT: mcpRoot, + OPENPENCIL_MCP_APP_TIMEOUT_MS: String(MCP_APP_ATTACH_TIMEOUT_MS) } }) diff --git a/tests/engine/mcp/server/watchdog.test.ts b/tests/engine/mcp/server/watchdog.test.ts new file mode 100644 index 000000000..0c0cd1bcf --- /dev/null +++ b/tests/engine/mcp/server/watchdog.test.ts @@ -0,0 +1,161 @@ +import { afterEach, describe, expect, test } from 'bun:test' +import { access } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' + +import { SceneGraph } from '@open-pencil/scene-graph' + +import { startServer, type ServerHandle } from '#mcp/server' +import { getDiscoveryPath } from '#mcp/transport/paths' + +import { connectMockBrowser, type HealthResponse } from '#tests/helpers/mcp/server' + +// Issue #488: an MCP server spawned by the app but never claimed by one +// (renderer crash, forced reload) used to squat its port forever with a +// stale discovery file. appAttachTimeoutMs closes such a server automatically. + +const TEST_AUTH_TOKEN = 'test-auth-token' +let testCounter = 0 + +function testSocketPath(): string | null { + if (process.platform === 'win32') return null + return join(tmpdir(), `openpencil-test-watchdog-${process.pid}-${++testCounter}.sock`) +} + +async function fileExists(path: string): Promise { + return access(path) + .then(() => true) + .catch(() => false) +} + +function sleep(ms: number): Promise { + return new Promise((resolve) => { + setTimeout(resolve, ms) + }) +} + +async function waitForHealthStatus(port: number, status: HealthResponse['status']): Promise { + for (let attempt = 0; attempt < 20; attempt++) { + const response = await fetch(`http://127.0.0.1:${port}/health`) + const health = (await response.json()) as HealthResponse + if (health.status === status) return + await sleep(5) + } + throw new Error(`Health endpoint did not report ${status}`) +} + +describe('MCP app-attach watchdog', () => { + let handle: ServerHandle | null = null + + afterEach(async () => { + await handle?.close().catch(() => undefined) + handle = null + }) + + test('closes the server and removes its discovery file when no app registers in time', async () => { + handle = await startServer({ + httpPort: 0, + withTcp: true, + socketPath: testSocketPath(), + authToken: TEST_AUTH_TOKEN, + enableEval: false, + mcpRoot: null, + appAttachTimeoutMs: 50 + }) + const port = handle.httpPort + expect(port).toBeGreaterThan(0) + + const discoveryPath = await getDiscoveryPath() + expect(await fileExists(discoveryPath)).toBe(true) + + // Nobody ever registers as the app. Wait past the watchdog timeout. + await sleep(300) + + await expect(fetch(`http://127.0.0.1:${port}/health`)).rejects.toThrow() + expect(await fileExists(discoveryPath)).toBe(false) + + handle = null // already closed by the watchdog — afterEach must not double-close + }) + + test('keeps a registered app alive past the initial grace period', async () => { + handle = await startServer({ + httpPort: 0, + withTcp: true, + socketPath: testSocketPath(), + authToken: TEST_AUTH_TOKEN, + enableEval: false, + mcpRoot: null, + appAttachTimeoutMs: 100 + }) + const port = handle.httpPort + + const browser = await connectMockBrowser(port, new SceneGraph(), TEST_AUTH_TOKEN) + try { + // Wait past the watchdog timeout — a registered app must not be evicted. + await sleep(300) + + const health = await fetch(`http://127.0.0.1:${port}/health`) + expect(health.status).toBe(200) + const data = (await health.json()) as HealthResponse + expect(data.status).toBe('ok') + } finally { + browser.close() + } + }) + + test('closes the server after a registered app disconnects for the grace period', async () => { + handle = await startServer({ + httpPort: 0, + withTcp: true, + socketPath: testSocketPath(), + authToken: TEST_AUTH_TOKEN, + enableEval: false, + mcpRoot: null, + appAttachTimeoutMs: 75 + }) + const port = handle.httpPort + + const browser = await connectMockBrowser(port, new SceneGraph(), TEST_AUTH_TOKEN) + browser.close() + await sleep(300) + + await expect(fetch(`http://127.0.0.1:${port}/health`)).rejects.toThrow() + handle = null + }) + + test('cancels pending shutdown when the app reconnects', async () => { + handle = await startServer({ + httpPort: 0, + withTcp: true, + socketPath: testSocketPath(), + authToken: TEST_AUTH_TOKEN, + enableEval: false, + mcpRoot: null, + appAttachTimeoutMs: 150 + }) + const port = handle.httpPort + + const firstBrowser = await connectMockBrowser(port, new SceneGraph(), TEST_AUTH_TOKEN) + firstBrowser.close() + await waitForHealthStatus(port, 'no_app') + const secondBrowser = await connectMockBrowser(port, new SceneGraph(), TEST_AUTH_TOKEN) + try { + await sleep(250) + const health = await fetch(`http://127.0.0.1:${port}/health`) + expect(health.status).toBe(200) + const data = (await health.json()) as HealthResponse + expect(data.status).toBe('ok') + } finally { + secondBrowser.close() + } + }) + + test('rejects timer values that cannot be represented safely', async () => { + await expect( + startServer({ appAttachTimeoutMs: 2_147_483_648, socketPath: testSocketPath() }) + ).rejects.toThrow('appAttachTimeoutMs must be in the range 0–2147483647') + await expect( + startServer({ appAttachTimeoutMs: Number.POSITIVE_INFINITY, socketPath: testSocketPath() }) + ).rejects.toThrow('appAttachTimeoutMs must be a safe integer') + }) +}) diff --git a/tests/engine/tauri/mcp-spawn.test.ts b/tests/engine/tauri/mcp-spawn.test.ts index 7ab16c8c7..8ef1f98ce 100644 --- a/tests/engine/tauri/mcp-spawn.test.ts +++ b/tests/engine/tauri/mcp-spawn.test.ts @@ -137,7 +137,8 @@ describe('Tauri MCP spawning', () => { OPENPENCIL_MCP_AUTH_TOKEN: expect.any(String), OPENPENCIL_MCP_CORS_ORIGIN: 'tauri://localhost', OPENPENCIL_MCP_TCP: '1', - OPENPENCIL_MCP_ROOT: '/mock/home' + OPENPENCIL_MCP_ROOT: '/mock/home', + OPENPENCIL_MCP_APP_TIMEOUT_MS: '30000' } } })