-
Notifications
You must be signed in to change notification settings - Fork 5
fix: exit orphaned stdio MCP servers on parent death #18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
jack-arturo
wants to merge
4
commits into
main
Choose a base branch
from
fix/stdio-orphan-exit
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+203
−12
Open
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
f810af1
fix: exit orphaned stdio MCP servers on parent death
jack-arturo 59c8480
fix: make soft stdin EOF a true no-op
jack-arturo 693df87
fix: pin parent pid before Local detection; add soft-EOF tests
jack-arturo 8dbed2d
fix: avoid mock.timers in lifecycle tests for Node 18
jack-arturo File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,128 @@ | ||
| /** | ||
| * Parent-liveness watchdog for the stdio MCP server. | ||
| * | ||
| * When an intermediate wrapper (npx → npm exec → node bin → server) keeps the | ||
| * server's stdin write-end open, a dead client never delivers EOF, so the leaf | ||
| * sits in the event loop forever and can thrash swap (multi-GB). Soft stdin | ||
| * EOF alone is not enough to catch that orphan case. The watchdog catches it | ||
| * by noticing the original parent is gone. | ||
| * | ||
| * Soft stdin EOF is intentional: do NOT call `transport.close()` and do NOT | ||
| * `process.exit` on `end`/`close`. Closing the transport trips the MCP SDK | ||
| * `onclose` → hard exit and breaks one-shot `echo … | npm start` flows. | ||
| * Hard exit is reserved for parent-watchdog reparent + OS signals. | ||
| * | ||
| * Kept side-effect-free on import so unit tests can cover it without spawning | ||
| * the full server. | ||
| */ | ||
|
|
||
| export type ParentLivenessProbe = (parentPid: number) => boolean; | ||
|
|
||
| /** Poll interval (ms) used when the env override is unset or invalid. */ | ||
| export const DEFAULT_PARENT_WATCHDOG_MS = 30_000; | ||
|
|
||
| const MIN_PARENT_WATCHDOG_MS = 100; | ||
|
|
||
| /** | ||
| * Parse LOCALWP_PARENT_WATCHDOG_MS into a safe poll interval. | ||
| * | ||
| * Non-finite / zero / negative → default. No disable value: an unparseable | ||
| * knob must never silently turn orphan protection off. Floored at | ||
| * MIN_PARENT_WATCHDOG_MS so tiny inputs can't spin the CPU. | ||
| */ | ||
| export function parseWatchdogIntervalMs(raw: string | undefined): number { | ||
| const n = Number(raw); | ||
| if (!Number.isFinite(n) || n <= 0) return DEFAULT_PARENT_WATCHDOG_MS; | ||
| return Math.max(n, MIN_PARENT_WATCHDOG_MS); | ||
|
jack-arturo marked this conversation as resolved.
|
||
| } | ||
|
|
||
| /** | ||
| * Default probe: the server has been reparented away from its original parent. | ||
| * | ||
| * POSIX only: this relies on the kernel reparenting an orphan (to pid 1 or a | ||
| * subreaper) when its parent dies. Windows does not reparent orphans, so | ||
| * `process.ppid` never changes there and this probe never fires — the watchdog | ||
| * is a no-op on win32. Orphan mitigation on Windows would need a different | ||
| * mechanism (e.g. a job object or a stdin heartbeat). Matches mcp-automem. | ||
| */ | ||
| export function parentReparented(parentPid: number): boolean { | ||
| return process.ppid !== parentPid; | ||
|
jack-arturo marked this conversation as resolved.
|
||
| } | ||
|
|
||
| /** | ||
| * Poll for the original parent's death and invoke `onDead` exactly once. | ||
| * Interval is unref'd so the watchdog alone never keeps the event loop alive. | ||
| */ | ||
| export function startParentWatchdog( | ||
| parentPid: number, | ||
| intervalMs: number, | ||
| onDead: () => void, | ||
| isParentGone: ParentLivenessProbe = parentReparented | ||
| ): NodeJS.Timeout { | ||
| let fired = false; | ||
| const timer = setInterval(() => { | ||
| if (fired) return; | ||
| if (isParentGone(parentPid)) { | ||
| fired = true; | ||
| onDead(); | ||
| } | ||
| }, intervalMs); | ||
| timer.unref(); | ||
| return timer; | ||
| } | ||
|
|
||
| /** | ||
| * Install stdin/transport/signal/parent-watchdog shutdown hooks. | ||
| * Call before `server.connect(transport)`. | ||
| * | ||
| * If `main()` awaits anything before this call, pass `parentPid` captured | ||
| * synchronously at the top of `main()` — `process.ppid` is dynamic, and a | ||
| * late capture after reparent would make the watchdog a no-op (mcp-automem #137). | ||
| */ | ||
| export function installStdioLifecycle(options: { | ||
| transport: { close?: () => unknown }; | ||
| onCloseAssignable?: { onclose?: (() => void) | null }; | ||
| envName?: string; | ||
| onShutdown?: () => void; | ||
| /** Prefer a pid captured before any `await` in `main()`. */ | ||
| parentPid?: number; | ||
| }): () => void { | ||
| const parentPid = options.parentPid ?? process.ppid; | ||
| let shuttingDown = false; | ||
| const shutdown = (code = 0) => { | ||
| if (shuttingDown) return; | ||
| shuttingDown = true; | ||
| try { | ||
| options.onShutdown?.(); | ||
| } catch { | ||
| /* best effort */ | ||
| } | ||
| try { | ||
| void options.transport.close?.(); | ||
| } catch { | ||
| /* best effort */ | ||
| } | ||
| process.exit(code); | ||
| }; | ||
|
|
||
| // Soft stdin EOF → no-op. Do not transport.close() (SDK onclose → hard exit) | ||
| // and do not process.exit. One-shot pipes must drain in-flight handlers; | ||
| // orphans that keep stdin open are killed by the parent watchdog / signals. | ||
| process.stdin.on('end', () => {}); | ||
| process.stdin.on('close', () => {}); | ||
| if (options.onCloseAssignable) { | ||
| options.onCloseAssignable.onclose = () => shutdown(0); | ||
| } | ||
| for (const sig of ['SIGTERM', 'SIGINT', 'SIGHUP'] as const) { | ||
| process.on(sig, () => shutdown(0)); | ||
| } | ||
|
|
||
| const envName = options.envName ?? 'LOCALWP_PARENT_WATCHDOG_MS'; | ||
| startParentWatchdog( | ||
| parentPid, | ||
| parseWatchdogIntervalMs(process.env[envName]), | ||
| () => shutdown(0) | ||
| ); | ||
|
|
||
| return shutdown; | ||
| } | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| import assert from 'node:assert/strict'; | ||
| import { afterEach, describe, it, mock } from 'node:test'; | ||
| import { | ||
| DEFAULT_PARENT_WATCHDOG_MS, | ||
| installStdioLifecycle, | ||
| parseWatchdogIntervalMs, | ||
| startParentWatchdog, | ||
| } from '../src/lifecycle.ts'; | ||
|
|
||
| describe('parseWatchdogIntervalMs', () => { | ||
| it('defaults on unset/invalid/non-positive', () => { | ||
| assert.equal(parseWatchdogIntervalMs(undefined), DEFAULT_PARENT_WATCHDOG_MS); | ||
| assert.equal(parseWatchdogIntervalMs(''), DEFAULT_PARENT_WATCHDOG_MS); | ||
| assert.equal(parseWatchdogIntervalMs('nope'), DEFAULT_PARENT_WATCHDOG_MS); | ||
| assert.equal(parseWatchdogIntervalMs('0'), DEFAULT_PARENT_WATCHDOG_MS); | ||
| assert.equal(parseWatchdogIntervalMs('-1'), DEFAULT_PARENT_WATCHDOG_MS); | ||
| }); | ||
|
|
||
| it('honours positive values floored at 100ms', () => { | ||
| assert.equal(parseWatchdogIntervalMs('250'), 250); | ||
| assert.equal(parseWatchdogIntervalMs('50'), 100); | ||
| }); | ||
| }); | ||
|
|
||
| describe('startParentWatchdog', () => { | ||
| it('fires once when the probe reports parent gone', async () => { | ||
| let calls = 0; | ||
| const timer = startParentWatchdog(1, 100, () => { | ||
| calls += 1; | ||
| }, () => true); | ||
| assert.equal(calls, 0); | ||
| await new Promise((r) => setTimeout(r, 150)); | ||
| assert.equal(calls, 1); | ||
| await new Promise((r) => setTimeout(r, 250)); | ||
| assert.equal(calls, 1); | ||
| clearInterval(timer); | ||
| }); | ||
| }); | ||
|
|
||
| describe('installStdioLifecycle soft stdin EOF', () => { | ||
| afterEach(() => { | ||
| mock.restoreAll(); | ||
| }); | ||
|
|
||
| it('does not close transport or exit on stdin end/close', () => { | ||
| const transport = { close: mock.fn() }; | ||
| const exitMock = mock.method(process, 'exit', () => undefined as never); | ||
|
|
||
| installStdioLifecycle({ | ||
| transport, | ||
| parentPid: process.ppid, | ||
| envName: 'LOCALWP_TEST_PARENT_WATCHDOG_MS', | ||
| }); | ||
|
|
||
| process.stdin.emit('end'); | ||
| process.stdin.emit('close'); | ||
|
|
||
| assert.equal(transport.close.mock.callCount(), 0); | ||
| assert.equal(exitMock.mock.callCount(), 0); | ||
| }); | ||
| }); |
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.