fix: shut down on stdin EOF and SIGHUP to prevent zombie processes - #44
Merged
Merged
Conversation
StdioServerTransport in @modelcontextprotocol/sdk does not listen for stdin 'close'/'end' events (upstream issue modelcontextprotocol/typescript-sdk#2002). When the MCP client (opencode, Claude Desktop, Cursor, etc.) disconnects without delivering SIGTERM — common on terminal close, IDE crash, or process kill — the figma-bridge server never notices and keeps running indefinitely. The election setInterval keeps the Node event loop alive, so the process becomes a zombie until manually killed or the machine reboots. Add shutdown handlers for: - stdin end/close (MCP stdio shutdown convention; cross-platform signal that does not depend on the client sending SIGTERM, which is not delivered on Windows and not sent at all when only the pipe closes) - SIGHUP (terminal/parent hangup) - uncaughtException (defensive — log + shutdown) - unhandledRejection (log only; a stray rejection should not kill the server, matching chrome-devtools-mcp's behavior) Shutdown is async-idempotent with a 5s force-exit backstop (unref'd) in case bridge/HTTP cleanup stalls. SIGINT/SIGTERM handlers preserved. Idle timeout is deliberately NOT added: a top-level MCP server that self-kills on idle leaves its client with a dead pipe and (in clients like opencode) no respawn path, so the tools silently vanish until a full client restart. The stdin EOF path already covers every real exit scenario. References: - modelcontextprotocol/typescript-sdk#2002 - ChromeDevTools/chrome-devtools-mcp (same pattern, src/bin/chrome-devtools-mcp-main.ts) - openreplay/openreplay (same pattern, mcp_app/server.ts)
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesServer lifecycle handling
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/src/index.ts`:
- Around line 23-31: Update the shutdown handler around election.stop() and
node.stop() to await the StdioServerTransport.close() promise before completing
normal shutdown, and remove the immediate process.exit(0) from that success
path. Retain the existing force timeout as the fallback exit, and route
transport-close failures to the failure path that exits appropriately.
- Around line 41-44: Update shutdown to accept an exit code, preserving its
existing default behavior, and pass 1 from the uncaughtException handler so that
path exits with failure status instead of process.exit(0). Keep other shutdown
callers unchanged unless they explicitly require a different status.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Address CodeRabbit feedback: 1. Await StdioServerTransport.close() before process.exit so the SDK's async cleanup (listener removal, onclose callback, McpServer internal cleanup) completes. The 5s force-exit backstop is retained as the timeout fallback; process.exit is now only the success path after cleanup or the failure path after timeout. 2. shutdown() now accepts an exit code (default 0). The uncaughtException handler passes 1 so the process reports failure status to any supervisor instead of masquerading as a clean exit. The transport reference is captured before shutdown is registered so the closure can reach it; startup races where shutdown fires before the transport exists are guarded by an explicit null check.
Contributor
|
Thanks for the contrib @dev-hann will be part of |
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.
Problem
figma-mcp-bridgeaccumulates zombie processes. When the MCP client (opencode, Claude Desktop, Cursor, etc.) disconnects without delivering SIGTERM — common on terminal close, IDE crash, orkill -9— the server never notices and keeps running indefinitely. The electionsetIntervalkeeps the Node event loop alive, so the process survives until manually killed or the machine reboots.Real-world observation: two orphaned
figma-mcp-bridgeprocesses found consuming memory after separate opencode sessions ended normally.Root Cause
StdioServerTransportin@modelcontextprotocol/sdkonly registers listeners for stdindataanderror— notcloseorend(upstream issue modelcontextprotocol/typescript-sdk#2002, labeled P1, fix proposed, still open).Combined with the always-on election
setInterval(server/src/election.ts:28, 3–5s), the event loop never drains, so even though the stdin pipe closes when the client dies, the server never exits.SIGTERM-only handling is insufficient:
close/endevent is the only reliable cross-platform shutdown signal.Solution
Add shutdown handlers in
server/src/index.ts(Pattern A, matchingchrome-devtools-mcpandopenreplay):process.stdin.on("end" | "close", shutdown)process.on("SIGHUP", shutdown)process.on("SIGINT" | "SIGTERM", shutdown)process.on("uncaughtException", shutdown)process.on("unhandledRejection", log)Shutdown is async-idempotent with a 5s force-exit backstop (unref'd) in case bridge/HTTP cleanup stalls.
What this does NOT do (deliberately)
No idle timeout. A top-level MCP server that self-kills on idle leaves its client with a dead pipe and no respawn path. For example, opencode's MCP client (
packages/opencode/src/mcp/index.ts) marks the serverfailedon pipe close and never respawns — tools silently vanish until a full client restart. stdin EOF already covers every real exit scenario, so idle reaping only hurts UX.(Idle reaping only makes sense for "bridge child" architectures where a long-lived parent spawns short-lived per-context children — see
mksglu/context-mode'slifecycle.ts, which gates the idle reaper onCONTEXT_MODE_BRIDGE_DEPTH>0and explicitly disables it for top-level servers.)Testing
No test framework configured (
server/package.jsonhas onlybuildandprepublishOnlyscripts). Manual integration tests:Test A — SIGINT (graceful regression):
✓ "Shutting down (SIGINT)..." printed, process exited cleanly.
Test B — stdin EOF (core fix):
✓ "Shutting down (stdin end)..." printed immediately, process exited.
Test C — parent SIGKILL (real-world scenario):
✓ Child detected stdin EOF, printed "Shutting down (stdin end)...", exited.
pgrepconfirmed no surviving processes.(TypeScript build also clean:
npm run buildsucceeds with no errors.)References
chrome-devtools-mcp: https://github.com/ChromeDevTools/chrome-devtools-mcp/blob/main/src/bin/chrome-devtools-mcp-main.tsopenreplay: https://github.com/openreplay/openreplay/blob/main/mcp_app/server.tsChecklist
npm run buildpassesSummary by CodeRabbit