Skip to content

feat(server): add host process watchdog to StdioServerTransport #1712

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.

Sign up for GitHub

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
travisbreaks wants to merge 2 commits into
base: main
Choose a base branch
from
+57 −1
Open

feat(server): add host process watchdog to StdioServerTransport #1712

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0bcf8fc
refactor: simplify to stdin EOF handling per review feedback
travisbreaks Apr 10, 2026
513efd2
Merge branch 'main' into feat/server-orphan-detection
KKonstantinov May 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/server-host-watchdog.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@modelcontextprotocol/server': patch
---

Close StdioServerTransport when stdin reaches EOF. When the host process exits or is killed, the stdin pipe closes and the transport now detects this via the `end` event, preventing orphaned server processes. Aligns with the behavior of the Python and Kotlin SDKs.

Check warning on line 5 in .changeset/server-host-watchdog.md

View check run for this annotation

Claude / Claude Code Review

PR title/description describe abandoned watchdog approach, not the shipped stdin-EOF implementation 

The PR title ("add host process watchdog") and description (clientProcessId, watchdogIntervalMs, `process.kill(pid, 0)` polling, `StdioServerTransportOptions`, constructor overloads, `unref()` timer, and the listed test plan) still describe the original implementation that was replaced on 2026-04-07 with the stdin-EOF approach — none of those exist in the shipped diff. Please update the PR title/body before merge so the squash-commit message matches what actually shipped; the changeset filename
Comment on lines +1 to +5
Copy link
Copy Markdown

@claude claude Bot May 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 The PR title ("add host process watchdog") and description (clientProcessId, watchdogIntervalMs, process.kill(pid, 0) polling, StdioServerTransportOptions, constructor overloads, unref() timer, and the listed test plan) still describe the original implementation that was replaced on 2026-04-07 with the stdin-EOF approach — none of those exist in the shipped diff. Please update the PR title/body before merge so the squash-commit message matches what actually shipped; the changeset filename server-host-watchdog.md is also a stale leftover (cosmetic, since changeset files are deleted on release).

Extended reasoning...

What's wrong

The PR metadata describes an implementation that no longer exists. The title is "feat(server): add host process watchdog to StdioServerTransport" and the description documents:

  • a clientProcessId option
  • a watchdogIntervalMs option (default 3s)
  • polling via process.kill(pid, 0)
  • a new StdioServerTransportOptions interface
  • backwards-compatible constructor overloads
  • a timer with .unref()
  • a test plan listing should close transport when host process is gone, should not close when host process is alive, should stop watchdog on close, should accept options object constructor

Per the timeline (author comments on 2026-04-05 and 2026-04-07), this was completely rewritten to the stdin-EOF approach. The shipped diff contains none of the above.

What actually shipped

packages/server/src/server/stdio.ts adds exactly three lines of behavior:

_onend = () => void this.close().catch(error => this.onerror?.(error));
// ...
this._stdin.on('end', this._onend);   // in start()
this._stdin.off('end', this._onend);  // in close()

No options, no interface, no constructor overload, no timer, no PID polling. The tests are should close transport when stdin emits end, should not fire onclose twice when close() is called after stdin end, and should remove end listener on close — none of the four tests listed in the PR description exist.

Step-by-step proof

  1. PR description, bullet 1: "Adds clientProcessId option" → grep clientProcessId packages/server/src/server/stdio.ts → no matches.
  2. PR description, bullet 4: "Configurable check interval via watchdogIntervalMs" → grep watchdogIntervalMs → no matches.
  3. PR description, bullet 5: "New StdioServerTransportOptions interface" → grep StdioServerTransportOptions → no matches. (The earlier bot review flagging an unused import type { StdioServerTransportOptions } is also obsolete — that import no longer exists in the test file.)
  4. PR description, bullet 6: "Timer uses unref()" → grep unref → no matches.
  5. Test plan item 1: should close transport when host process is gone → not in stdio.test.ts; the actual test is should close transport when stdin emits end.
  6. Changeset body (.changeset/server-host-watchdog.md): "Close StdioServerTransport when stdin reaches EOF … detects this via the end event" — this is correct and matches the code. Only the filename is stale.

Why it matters

This repo squash-merges PRs, so the PR title and body become the permanent commit message on main. Anyone running git log or git blame on stdio.ts will see a commit claiming to add a PID-polling watchdog with a configurable interval and a new options interface — and find none of it in the tree. The repo's review conventions explicitly call out "prose that promises behavior the code no longer ships" as something to flag.

The published npm changelog will be fine (the changeset body is accurate), so this has zero runtime or API impact — hence nit severity.

Fix

Before merge:

  • Retitle to something like feat(server): close StdioServerTransport on stdin EOF to prevent orphaned processes.
  • Replace the PR description's Summary/Test plan with the stdin-EOF behavior and the three actual test names.
  • Optionally rename .changeset/server-host-watchdog.md → something like .changeset/server-stdin-eof.md (cosmetic — changeset files are deleted on release, so this only affects git history).

5 changes: 4 additions & 1 deletion packages/server/src/server/stdio.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,11 @@ export class StdioServerTransport implements Transport {
_onerror = (error: Error) => {
this.onerror?.(error);
};
_onend = () => void this.close().catch(error => this.onerror?.(error));
_onstdouterror = (error: Error) => {
this.onerror?.(error);
this.close().catch(() => {
// Ignore errors during close — we're already in an error path
// Ignore errors during close -- we're already in an error path
});
};

Expand All @@ -58,6 +59,7 @@ export class StdioServerTransport implements Transport {
this._started = true;
this._stdin.on('data', this._ondata);
this._stdin.on('error', this._onerror);
this._stdin.on('end', this._onend);
this._stdout.on('error', this._onstdouterror);
}

Expand Down Expand Up @@ -85,6 +87,7 @@ export class StdioServerTransport implements Transport {
// Remove our event listeners first
this._stdin.off('data', this._ondata);
this._stdin.off('error', this._onerror);
this._stdin.off('end', this._onend);
this._stdout.off('error', this._onstdouterror);

// Check if we were the only data listener
Expand Down
48 changes: 48 additions & 0 deletions packages/server/test/server/stdio.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,3 +179,51 @@ test('should fire onerror before onclose on stdout error', async () => {

expect(events).toEqual(['error', 'close']);
});

describe('stdin EOF', () => {
test('should close transport when stdin emits end', async () => {
const server = new StdioServerTransport(input, output);

const closed = new Promise<void>(resolve => {
server.onclose = () => resolve();
});

await server.start();

// Simulate the host process closing stdin (pipe EOF)
input.push(null);

await closed;
});

test('should not fire onclose twice when close() is called after stdin end', async () => {
const server = new StdioServerTransport(input, output);

let closeCount = 0;
server.onclose = () => {
closeCount++;
};

await server.start();
input.push(null);

// Give the end event time to propagate
await new Promise(resolve => setTimeout(resolve, 50));

await server.close();
expect(closeCount).toBe(1);
});

test('should remove end listener on close', async () => {
const server = new StdioServerTransport(input, output);
await server.start();

const endListenersBefore = input.listenerCount('end');
expect(endListenersBefore).toBeGreaterThan(0);

await server.close();

const endListenersAfter = input.listenerCount('end');
expect(endListenersAfter).toBe(0);
});
});
Loading