fix(mcp): close orphaned servers after app disconnects (#494)

* fix(mcp): close orphaned servers that no app ever claims

- Add ServerOptions.appAttachTimeoutMs: if no app registers within this
  window after startup, the server closes itself and removes its
  discovery file, instead of squatting the port indefinitely.
- Wire it through the openpencil-mcp-http CLI as
  OPENPENCIL_MCP_APP_TIMEOUT_MS (opt-in, unset/0 disables it — a bare
  CLI invocation for manual testing should not self-terminate).
- The desktop app opts in with a 30s timeout when it spawns the server.

Without this, a server that outlives its spawning app (renderer crash,
forced reload) keeps holding its port with a stale discovery file. The
app's liveness check only asks whether something answers /health, not
whether an app has ever registered (see /health's no_app status) — so
every later launch finds the orphan already listening and defers to
it, and MCP tool calls fail with "app is not connected" until someone
manually kills the orphaned process. Closing self-caused orphans at
the source means the next launch finds no discovery file and takes
the normal fresh-spawn path.

Fixes #488

* fix(mcp): clean up servers after app disconnects

- Re-arm the orphan watchdog when the registered app disconnects

- Cancel pending shutdown when the app reconnects within the grace period

- Reject timeout values that overflow the runtime timer range

* test(mcp): make watchdog reconnect coverage deterministic

- Wait for the disconnected health state before reconnecting

- Report distinct safe-integer and timer-range validation errors

---------

Co-authored-by: swe-sanad <sanad.arousi@export119.com>

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<WebSocket>()
+  const connectionListeners = new Set<ConnectionListener>()
   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,

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`)

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<ServerHandle> {
+  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<ServerHa
   const resolvedSocketPath = state.socketResult?.resolvedPath ?? null
   const actualHttpPort = state.tcpResult?.port ?? 0
 
-  return buildHandle(
+  const handle = buildHandle(
     ctx.app,
     ctx.wss,
     ctx.browserRPC,
@@ -440,4 +452,59 @@ export async function startServer(options: ServerOptions = {}): Promise<ServerHa
     ctx.authToken,
     startedAt
   )
+
+  armAppAttachWatchdog(options.appAttachTimeoutMs, ctx.browserRPC, handle)
+
+  return handle
+}
+
+const MAX_TIMER_MS = 2_147_483_647
+
+function validateAppAttachTimeout(timeoutMs: number | undefined): void {
+  if (timeoutMs === undefined) return
+  if (!Number.isSafeInteger(timeoutMs)) {
+    throw new RangeError('appAttachTimeoutMs must be a safe integer')
+  }
+  if (timeoutMs < 0 || timeoutMs > 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<typeof createBrowserRPCBridge>,
+  handle: ServerHandle
+): void {
+  if (timeoutMs === undefined || timeoutMs === 0) return
+
+  let timer: ReturnType<typeof setTimeout> | 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()
+  })
 }

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<AutomationServerHandle | null> {
       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)
     }
   })
 

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<boolean> {
+  return access(path)
+    .then(() => true)
+    .catch(() => false)
+}
+
+function sleep(ms: number): Promise<void> {
+  return new Promise((resolve) => {
+    setTimeout(resolve, ms)
+  })
+}
+
+async function waitForHealthStatus(port: number, status: HealthResponse['status']): Promise<void> {
+  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')
+  })
+})

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'
             }
           }
         })