[DoroteoNavarrete]

On Windows, transport:stdio:shutdown-escalation no longer requires the child to log that it ignored SIGTERM; it still requires close() to leave no live child. Case 3 of #3117.

Motivation and Context

The scenario fails on Windows because it waits for the fixture to log [stdio-server] sigterm ignored. Windows has no POSIX signals: per the Node docs, subprocess.kill('SIGTERM') kills the process "forcefully and abruptly (similar to 'SIGKILL')", so the fixture never gets the chance to ignore it.

The transport is fine on Windows (the child does die). Only the test's premise is wrong there.

On win32 the test now skips only the marker check. The check that close() leaves no live child still runs on every platform, so on Windows the test still verifies something real. I also updated the comment above it, since "termination can only have come from SIGKILL" isn't true on Windows.

How Has This Been Tested?

Windows 11 Pro 26100, Node 24.11.1, pnpm 10.26.1:

• Before: pnpm --filter @modelcontextprotocol/test-e2e test → 2 failures (expected '[stdio-server] ready\n' to contain '[stdio-server] sigterm ignored').
• After: 45 files, 3126 passed, 141 expected fail, 0 failures.
• Mutation check: with both kill() calls in close() commented out, the test fails on Windows at processAlive(pid), so the remaining assertion still catches a broken shutdown.
• tsc --noEmit and ESLint pass for test/e2e.

Breaking Changes

None. Test-only change, so no changeset.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update

Checklist

• I have read the MCP Documentation
• My code follows the repository's style guidelines
• New and existing tests pass locally
• I have added appropriate error handling
• I have added or updated documentation as needed

Additional context

I didn't skip the whole scenario because verifies() has no per-platform skip (knownFailures filters by transport and spec version, not OS), and an early return would pass without testing anything. If you'd rather have platform support in knownFailures, I can do that instead.

Refs #3117

🤖 Generated with Claude Code

[changeset-bot]

⚠️ No Changeset found

Latest commit: 0bb204a

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[pkg-pr-new]

Open in StackBlitz

@modelcontextprotocol/client

npm i https://pkg.pr.new/@modelcontextprotocol/client@3123

@modelcontextprotocol/codemod

npm i https://pkg.pr.new/@modelcontextprotocol/codemod@3123

@modelcontextprotocol/core

npm i https://pkg.pr.new/@modelcontextprotocol/core@3123

@modelcontextprotocol/server

npm i https://pkg.pr.new/@modelcontextprotocol/server@3123

@modelcontextprotocol/server-legacy

npm i https://pkg.pr.new/@modelcontextprotocol/server-legacy@3123

@modelcontextprotocol/express

npm i https://pkg.pr.new/@modelcontextprotocol/express@3123

@modelcontextprotocol/fastify

npm i https://pkg.pr.new/@modelcontextprotocol/fastify@3123

@modelcontextprotocol/hono

npm i https://pkg.pr.new/@modelcontextprotocol/hono@3123

@modelcontextprotocol/node

npm i https://pkg.pr.new/@modelcontextprotocol/node@3123

commit: 0bb204a

[arhancanli]

Read the diff against packages/client/src/client/stdio.ts (close(), lines ~280-316 on main). The ladder is: stdin.end(), wait 2000 ms, kill('SIGTERM'), wait 2000 ms, kill('SIGKILL'). Your Node-docs reading is right that on Windows the SIGTERM rung already kills the child, so the marker can't appear there. I didn't run this on Windows.

One thing the Windows branch no longer proves: that the child survived rung 1 (stdin EOF). If the fixture exited on EOF, processAlive(pid) would still be false and the test would pass. The first 2000 ms wait gives a platform-independent signal. close() should take at least about 2 s on every platform, because even on Windows rung 2 is only reached after the stdin grace period. So you could record const t0 = performance.now() before await client.close() and assert performance.now() - t0 >= 1900 inside the win32 branch (or for all platforms). That keeps "survives stdin EOF" covered where the marker can't be checked. The 100 ms of slack is for timer rounding, so tune it if CI is noisy.

That's a suggestion; the PR is correct as written for what it claims.

[DoroteoNavarrete]

Good catch, thanks. You're right that on Windows the test no longer proved the child survived stdin EOF.

I checked it with a mutation: making the fixture exit on stdin end. The current version still passed on Windows. With the timing check it fails (expected 8.77 to be greater than or equal to 1900).

Added it for all platforms rather than only inside the win32 branch. It's a lower bound, so a slow runner can't make it flaky, and on POSIX it holds too (~4s there).

[arhancanli]

Read the updated diff: the lower bound outside the win32 branch is the right shape, and the mutation check (fixture exiting on stdin end now fails at 8.77 vs 1900) is the evidence that it bites. The 1900 leaves 100ms of margin under the 2s grace, which covers timer rounding, and since the timer starts after closeStarted it can't undershoot by more than clock jitter.

One thing I could not tell from the diff: is the 2s grace a constant exported from the transport, or a literal in stdio.ts? If it is exported, importing it (minus the margin) would stop the test silently weakening if someone shortens the grace period later; if it is a literal, the 1900 here is a second copy of that number. Which is it?

[DoroteoNavarrete]

It's a literal: both rungs in close() use setTimeout(resolve, 2000) (stdio.ts lines 297 and 306), and nothing is exported or configurable.

I'd rather not export it. The e2e test imports from the public @modelcontextprotocol/client/stdio entry, so it would become public API just for a test. And if someone shortens the grace period, this assertion fails rather than silently weakening, so it would get noticed. If a maintainer prefers a shared constant, happy to add it.

[arhancanli]

Fair, keeping the 2s literal private makes sense, and a failing lower-bound assertion on a shortened grace period does get noticed. One thing I would check in the test: is the elapsed time measured with a monotonic clock (performance.now()) rather than Date.now()? On a loaded CI runner a wall-clock step could make the >= 2000 ms bound flaky in either direction.

[DoroteoNavarrete]

Yes, it uses \performance.now()\ (monotonic), and the bound is 1900 ms.

[arhancanli]

Good, performance.now() with the 1900 floor is the right pairing. One thing left that I could see in the diff: the bound is lower-only, so it proves the SIGKILL did not fire early but not that it fired at all within the grace window, and that part rests on the existing waitFor with its 2000 ms timeout. Since the first rung waits 2s and the second waits another 2s, would it be worth asserting an upper bound as well (for example under ~4500 ms) so a close() that hangs past both rungs fails on timing rather than on the later waitFor?

[DoroteoNavarrete]

I'd leave the upper bound out. Unlike the floor, a ceiling is exactly what a loaded runner can break while the code is fine, and the cases it would catch are already covered: a close() that hangs fails on the 15s scenario timeout from verifies(), and one that returns with the child still alive fails on the processAlive waitFor. So it would only change the failure message.

Small note: the floor shows the child survived stdin EOF (rung 1), not anything about when SIGKILL fired.

I'd like to keep this PR scoped to the Windows fix. Thanks for the careful reviews!

[arhancanli]

Agreed on the ceiling: a failure message is the only thing it would add, and a loaded runner can trip it with correct code. And thanks for the note on the floor: it shows rung 1 survived stdin EOF, not when SIGKILL fired, so I will read it that way. One question on the Windows side: with no SIGTERM there, does the processAlive waitFor timeout leave enough slack for slow process teardown on the hosted Windows runners, or have you seen it close to the limit in the runs so far?

[DoroteoNavarrete]

Plenty of slack. The repo's CI has no Windows runners (only ubuntu-latest, which is part of #3117), so these are local numbers: Windows 11, 10 runs with temporary timing around the assertions.

• close() took 2008–2024 ms.
• The child was already dead when close() returned, in all 10 runs.
• The processAlive waitFor resolved in 0.1–0.4 ms.

That's expected: on Windows kill('SIGTERM') terminates the child right away, and rung 2 races the child's close event, so close() only returns once the process has exited. The 2000 ms timeout is effectively unused there.

[arhancanli]

Those numbers settle it for me: 2008-2024 ms for close() and a child already dead on return means the rung-2 race is what ends it, so the processAlive timeout is not carrying anything on Windows. Keeping the PR scoped is fine. When #3117 adds a Windows runner, would you re-run the same 10-run timing there? Hosted runners are usually slower at process teardown than a local Windows 11 machine, and the floor of 1900 ms should hold either way.

[DoroteoNavarrete]

Sure. A Windows job isn't decided yet (it's an open question in #3117), but if one is added I'll rerun the timing there.