Skip to content

fix: close owned stdio sessions when stopping - #434

Merged
punkpeye merged 2 commits into
punkpeye:mainfrom
remote-controlled-man:codex/fastmcp-stdio-stop-01a13e77
Oct 17, 2026
Merged

punkpeye merged 2 commits into
punkpeye:mainfrom
remote-controlled-man:codex/fastmcp-stdio-stop-01a13e77

Conversation

@remote-controlled-man

Copy link
Copy Markdown
Contributor

stop() now closes the sessions and transports created by start({ transportType: "stdio" }). Previously it set the server state to stopped while the stdio session remained tracked and continued answering MCP requests.

Owned sessions use the existing close path, which cancels running tools, removes stdin listeners and emits disconnect. Stopping while session setup is waiting and failures while connecting now close the owned session and transport. Transports supplied through connect() retain their documented caller ownership. This complements #265, which handles the client closing stdin.

Validation on Windows with Node 22.20.0 and pnpm 10.32.1:

  • Whole-tree Prettier, ESLint, TypeScript and the package build pass. All 1,035 offline tests outside the separately run DiskStore file pass.
  • All four new lifecycle regressions fail on the unmodified base and pass with the fix. They cover shutdown/cancellation, repeated stop, mixed caller-owned and owned sessions, stopping during setup, and failed setup.
  • A real SDK StdioClientTransport subprocess probe confirms the base still answers ping after stop(); with the fix the session is removed, one disconnect is emitted, and ping fails on the closed connection.
  • The separate DiskStore test run reports 14 passed, 2 skipped and the same two concurrent-rename failures already observed on the unmodified base. They are not reported as passing.
  • The JSR dry-run with the existing Deno 1.41.3 reports the same ten dependency declaration errors on the base and this change. Linux/Node 24 CI has not been run locally.

AI assistance was used to prepare this change.

Copilot AI balanced review requested due to automatic review settings October 15, 2026 23:29

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@Dcastroro

Copy link
Copy Markdown
Contributor

Independent regression check on Windows / Node 24.20.0 at head dfeb7f0ab3cee6acaa0d790b86a21fdc78eaeb76:

  • The four new lifecycle tests fail with src/FastMCP.ts from base 913aa06de58abc651a64382fe2c17ccf7f1e85cf: owned sessions remain tracked, and the setup/failed-start paths do not close their transport.
  • Restoring the PR implementation makes all 27 tests pass across these files:
node node_modules/vitest/vitest.mjs run src/FastMCP.stdio-stop.test.ts src/FastMCP.stdio.test.ts src/FastMCP.in-memory.test.ts src/FastMCP.tool-cancellation.test.ts

The covered controls retain the caller-owned connection, reject ping on the stopped owned connection, abort the running tool, and emit one disconnect despite repeated stop. The implementation uses the existing session-close path and tracks only the sessions constructed by start().

This is focused regression evidence, using the new tests' in-memory replacement for stdio construction. I did not repeat the separate subprocess probe or run the full suite at this PR head. No provider credentials or external model calls were used.

AI assistance was used to review the diff and perform this validation.

@punkpeye

Copy link
Copy Markdown
Owner

Pushed a small fix: the SDK's close() pauses stdin, so a start({ transportType: "stdio" }) after stop() never read its client and the process exited. start() now resumes stdin when it is paused. Added a test that runs the real stdio transport over pipes. Thanks!

@punkpeye
punkpeye merged commit aabcce6 into punkpeye:main Oct 17, 2026
2 checks passed
@github-actions

Copy link
Copy Markdown

🎉 This PR is included in version 4.23.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants