fix(desktop): make embedded server start idempotent (#493) - #508
Draft
TYRMars wants to merge 1 commit into
Draft
Conversation
`ServerManager.startEmbedded()` unconditionally built a new Fastify server, AppState, and MCP child processes, overwriting `this.app` / `this.mcpClients` without closing the previous set. On macOS the app stays resident after the last window closes, so a dock re-activate re-enters createWindow() -> ensureServer() -> startEmbedded() with the server still running, orphaning the first generation (listener, MCP children, store handles) — `before-quit`/stop() only ever reaps the newest one. Guard the embedded branch: when an embedded server is already running for the same workspace, log and reuse it; when the workspace changed, stop the old one first (what restart() already does) before starting fresh. `restart()` still fully restarts because it calls stop() before reaching here. Adds a regression test asserting a second ensureServer() keeps the same origin (no second server on a new ephemeral port). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BjZhzgw54atKvgd9WnDunW
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #493.
ServerManager.startEmbedded()had no "already running" guard on the embedded branch, so a successful start could be silently replaced by a second one — leaking the first generation's Fastify listener, MCP child processes, and store handles.On macOS the app stays resident after the last window closes (
window-all-closeddoesn't quit on darwin). Clicking the dock icon re-enterscreateWindow()→ensureServer()→startEmbedded()while the server is still running. BecausepickPort()hands back a fresh ephemeral port there's noEADDRINUSEto surface the double start, andthis.app/this.mcpClientsget overwritten.before-quit→stop()then only ever reaps the newest generation, so every earlier server + MCP child survives until the user kills them by hand. WithJARVIS_DB_URL=sqlite:…a secondAppStatealso opens a second writer against the same DB file.Change
Guard the embedded branch in
startEmbedded():stop()the old one first (whatrestart()already does), then start fresh.restart()still performs a full restart because it callsstop()before reachingstartEmbedded(), sothis.appisnullthere and the guard is a no-op on that path.Testing
ensureServer({ forceEmbedded: true })after a successful embedded start keeps the sameapi_origin(proving no second server on a new port) and the single server stays healthy.pnpm --filter @jarvis/desktop typecheck✅node --test src/main/server-manager.test.ts— 3/3 pass ✅Notes
Not a duplicate of #170 (start-failure leak), #317 (lifecycle mutex — these calls are sequential), or #318 (
pickPortTOCTOU — the fresh port is what hides this bug). The missing piece was an idempotency check, which this adds.🤖 Generated with Claude Code
Generated by Claude Code