feat(server): add host process watchdog to StdioServerTransport#1712
feat(server): add host process watchdog to StdioServerTransport#1712travisbreaks wants to merge 1 commit intomodelcontextprotocol:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 6abe472 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
felixweinberger
left a comment
There was a problem hiding this comment.
Thanks. Please pull in a few things before merge (cherry-picks from #1815, credit @windro-xdd):
- Rebase onto main. Stale base conflicts with the
_closedguard and_onstdouterroradditions. - EPERM handling. In the watchdog catch, treat
err.code === 'EPERM'as still-alive. - Workerd shim. Add a
kill(): neverstub topackages/server/src/shimsWorkerd.ts. - Export
StdioServerTransportOptionsfrompackages/server/src/index.ts.
e253410 to
7d89648
Compare
|
@claude review |
|
|
||
| import type { JSONRPCMessage } from '@modelcontextprotocol/core'; | ||
| import { ReadBuffer, serializeMessage } from '@modelcontextprotocol/core'; | ||
|
|
There was a problem hiding this comment.
🟡 The import type { StdioServerTransportOptions } on line 6 of the test file is imported but never referenced anywhere in the file body. This will trigger an ESLint @typescript-eslint/no-unused-vars error since the project config applies that rule globally with no exception for type-only imports.
Extended reasoning...
The PR adds StdioServerTransportOptions as a new exported interface in packages/server/src/server/stdio.ts and also imports it in the test file on line 6 via import type { StdioServerTransportOptions } from '../../src/server/stdio.js'. However, the type is never actually used anywhere in the test file.
Searching the entire test file, no variable is declared with an explicit StdioServerTransportOptions type annotation. Every test case that exercises the new options-object constructor form passes inline object literals directly to new StdioServerTransport({...}) — for example, new StdioServerTransport({ stdin: input, stdout: output }) — relying on TypeScript structural type inference rather than an explicit annotation.
The project ESLint configuration (in common/eslint-config/eslint.config.mjs) sets @typescript-eslint/no-unused-vars: ["error", { argsIgnorePattern: "^_" }] globally. The only test-file-specific override disables unicorn/consistent-function-scoping, not no-unused-vars. The @typescript-eslint/no-unused-vars rule in typescript-eslint v8 does flag unused import type declarations, so this import will cause a lint error.
Concrete proof: scan every line of the test file for the string StdioServerTransportOptions — it appears only once, on the import line itself. There is no const opts: StdioServerTransportOptions = ..., no as StdioServerTransportOptions cast, no JSDoc reference, nothing.
The fix is straightforward: remove the unused import line. Since import type is erased at compile time, there is zero runtime impact; this is purely a lint/code-quality issue. Severity is nit — no functional regression, but it breaks the lint check.
There was a problem hiding this comment.
Thinking through this more deeply - while this is technically correct and would work, I wonder what the actual mechanism for using this would be without a change to the MCP spec.
By design this requires client and server to somehow agree how to exchange the processId. Because the spec doesn't have a standard method for it (LSP does, which is what the issue refers to as an example), it implicitly means client and server both need to be controlled by the developer 🤔 - it could still be useful for someone that does this but does raise the question of whether that justifies introducing new APIs for a transport that's probably less used than SHTTP. Without a lot of people calling for this I'd err on the side of not expanding the API surface unless we're sure people want this and run into this problem.
I also had a look at other SDKs and none currently do anything like this watchdog setup (not necessarily making this wrong, but a data point for demand).
However, the close problem does seem real, and there's something the other SDKs do that TS does not - that's handling stdin-EOF better.
I think we could probably get a lot of mileage if we just do something like this alongside the data/error listeners:
- Handler field (near
_ondata):
_onend = () => void this.close().catch(err => this.onerror?.(err));- In
start():
this._started = true;
this._stdin.on('data', this._ondata);
this._stdin.on('error', this._onerror);
this._stdin.on('end', this._onend); // ← add
this._stdout.on('error', this._onstdouterror);- In close() (near line 86, alongside the other .off() calls):
this._stdin.off('data', this._ondata);
this._stdin.off('error', this._onerror);
this._stdin.off('end', this._onend); // ← addWe currently don't listen for the end event but the other SDKs all do, so we'd close that gap here.
Would you be open to simplifying to this here and instead of testing the Watchdog setup testing process cleanup on an end event?
|
Thanks for the direction here. The stdin EOF approach is cleaner and I'm happy to rewrite to match what the other SDKs do. One question before I go: does stdin Either way, I'll rewrite to stdin EOF. Just want to understand if there's an edge case worth documenting or handling as a follow-up. |
Listen for the stdin `end` event to detect when the host process exits or is killed, and close the transport accordingly. This prevents orphaned server processes when the host terminates without cleanly shutting down the server. Replaces the previous watchdog approach with the simpler stdin EOF pattern used by the Python and Kotlin SDKs. Co-Authored-By: Tadao <tadao@travisfixes.com>
7d89648 to
6abe472
Compare
Summary
clientProcessIdoption toStdioServerTransportthat enables a watchdog timerprocess.kill(pid, 0))watchdogIntervalMs(default: 3s)StdioServerTransportOptionsinterface with backwards-compatible constructor overloadsunref()so it does not prevent clean process exitFollows the same pattern used in vscode-languageserver-node as suggested in the issue.
Closes #208
Test plan
should close transport when host process is gone: watchdog detects dead PID and closesshould not close when host process is alive: no false positives with own PIDshould stop watchdog on close: cleanup on transport closeshould accept options object constructor: new constructor form worksChangeset
@modelcontextprotocol/server: minor (new opt-in feature, no breaking changes)Co-Authored-By: Claude Opus 4.6 (1M context) tadao@travisfixes.com