Added clean shutdown + tests by jerome3o-anthropic · Pull Request #115 · modelcontextprotocol/typescript-sdk

jerome3o-anthropic · 2025-01-03T16:32:00Z

Motivation and Context

When closing a StdioServerTransport, the process would hang indefinitely due to unclosed stdin/stdout streams and lingering event listeners. This was particularly noticeable in server implementations that needed to shut down cleanly. The fix ensures proper cleanup of all resources, preventing process hangs.

How Has This Been Tested?

Added unit test in src/server/stdio.test.ts verifying proper cleanup of stream resources and event listeners
Added integration test in src/integration-tests/process-cleanup.test.ts that confirms the process exits cleanly after transport closure
Manually verified fix using a minimal reproduction case that previously demonstrated the hang

Breaking Changes

None. This is a bug fix that maintains the existing API contract.

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

The fix involves three key changes in StdioServerTransport.close():

Removing all event listeners, including 'drain' listeners that were previously overlooked
Explicitly destroying both stdin and stdout streams
Maintaining existing cleanup of the read buffer and onclose callback

The integration test was added to prevent regression, as unit tests alone didn't catch this issue due to the asynchronous nature of Node.js stream cleanup.

jspahrsummers

Thanks for working on this! Some blocking discussion, but this is definitely important

jspahrsummers · 2025-01-03T20:07:15Z

+ if (remainingDataListeners === 0) {
+ // Only pause stdin if we were the only listener
+ // This prevents interfering with other parts of the application that might be using stdin
+ this._stdin.pause();


I'm a bit surprised that this has any effect on termination, but then reading about Node.js readable streams, I'm like 😵
It feels a bit janky to do things this way, but I don't really have a better idea…

Per top-level PR comment, I don't see how both MCP and anything else can co-exist on the same stdin stream.

Certainly not while messages are streaming in. The only question is whether there'd ever be a reason for a server implementation to want to read from stdin after terminating the normal MCP stuff. Probably not? But it's always hard to anticipate how people will use a library.

davidfiala · 2025-01-03T23:38:37Z

I believe that since you are communicating a specific protocol over stdin/stdout, then it does assume you have permanently taken over both STDIO FDs.

This has interesting implications that I've had to work around. Specifically, wouldn't a console.log or console.info effectively corrupt your communication stream, if it occurs during any part of the server execution?

Here was an approximate workaround that I put at the top of servers to route the most common occurences to stderr instead:

// make console.log and console.info go to STDERR because MCP is going to take over STDOUTconstonlyStderr=newConsole({stdout: process.stderr,stderr: process.stderr});console.info=onlyStderr.info;console.log=onlyStderr.log;

In this case, I'd argue that yes it's fine/expected for you to end the stdin/stdout streams. It should be clearly documented that this behavior occurs IMHO. Likewise, perhaps you should hotpatch the console logs in your server.connect() on StdioServerTransport. The thing is, if the user happens to console.log BEFORE getting to your constructor, then there'd already be corruption-worthy output that the MCP client would be exposed to when it begins to read stdout. There's worse workarounds for that as well (ie, a shared sentinel to broadcast the true beginning of the MCP stream), but that is a breaking protocol change.

jspahrsummers · 2025-01-06T16:15:04Z

This has interesting implications that I've had to work around. Specifically, wouldn't a console.log or console.info effectively corrupt your communication stream, if it occurs during any part of the server execution?

Yes, this is discussed here: https://modelcontextprotocol.io/docs/tools/debugging#server-side-logging

Nedomas · 2025-01-08T00:36:09Z

Even though the fix here is for stdio specifically, I’m wondering isn’t the same leak happening in the SSE transport too?

I can’t seem to be getting rid of the active connection even with doing this:

constclient=newClient({name: 'superinterface-mcp-client',version: '1.0.0',},{capabilities: {}})consttransport=ewSSEClientTransport(newURL('some-url'))awaitclient.connect(transport)awaitclient.close()awaittransport.close()

But might be an unrelated bug?

jerome3o-anthropic · 2025-01-08T18:07:24Z

seem to be getting rid of the active connection even with doing

That's odd - likely a separate issue. Going to merge this now and follow up on the SSE bug later

…issue-114 fix(server) Avoid modifying path resolution for executables on MacOS

jerome3o-anthropic · 2025-01-03T16:32:00Z

Motivation and Context

#113

When closing a StdioServerTransport, the process would hang indefinitely due to unclosed stdin/stdout streams and lingering event listeners. This was particularly noticeable in server implementations that needed to shut down cleanly. The fix ensures proper cleanup of all resources, preventing process hangs.

How Has This Been Tested?

Added unit test in src/server/stdio.test.ts verifying proper cleanup of stream resources and event listeners
Added integration test in src/integration-tests/process-cleanup.test.ts that confirms the process exits cleanly after transport closure
Manually verified fix using a minimal reproduction case that previously demonstrated the hang

Breaking Changes

None. This is a bug fix that maintains the existing API contract.

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

The fix involves three key changes in StdioServerTransport.close():

Removing all event listeners, including 'drain' listeners that were previously overlooked
Explicitly destroying both stdin and stdout streams
Maintaining existing cleanup of the read buffer and onclose callback

The integration test was added to prevent regression, as unit tests alone didn't catch this issue due to the asynchronous nature of Node.js stream cleanup.

jspahrsummers

Thanks for working on this! Some blocking discussion, but this is definitely important

jspahrsummers · 2025-01-03T20:07:15Z

+ if (remainingDataListeners === 0) {
+ // Only pause stdin if we were the only listener
+ // This prevents interfering with other parts of the application that might be using stdin
+ this._stdin.pause();


I'm a bit surprised that this has any effect on termination, but then reading about Node.js readable streams, I'm like 😵
It feels a bit janky to do things this way, but I don't really have a better idea…

Per top-level PR comment, I don't see how both MCP and anything else can co-exist on the same stdin stream.

Certainly not while messages are streaming in. The only question is whether there'd ever be a reason for a server implementation to want to read from stdin after terminating the normal MCP stuff. Probably not? But it's always hard to anticipate how people will use a library.

davidfiala · 2025-01-03T23:38:37Z

I believe that since you are communicating a specific protocol over stdin/stdout, then it does assume you have permanently taken over both STDIO FDs.

This has interesting implications that I've had to work around. Specifically, wouldn't a console.log or console.info effectively corrupt your communication stream, if it occurs during any part of the server execution?

Here was an approximate workaround that I put at the top of servers to route the most common occurences to stderr instead:

// make console.log and console.info go to STDERR because MCP is going to take over STDOUTconstonlyStderr=newConsole({stdout: process.stderr,stderr: process.stderr});console.info=onlyStderr.info;console.log=onlyStderr.log;

In this case, I'd argue that yes it's fine/expected for you to end the stdin/stdout streams. It should be clearly documented that this behavior occurs IMHO. Likewise, perhaps you should hotpatch the console logs in your server.connect() on StdioServerTransport. The thing is, if the user happens to console.log BEFORE getting to your constructor, then there'd already be corruption-worthy output that the MCP client would be exposed to when it begins to read stdout. There's worse workarounds for that as well (ie, a shared sentinel to broadcast the true beginning of the MCP stream), but that is a breaking protocol change.

jspahrsummers · 2025-01-06T16:15:04Z

This has interesting implications that I've had to work around. Specifically, wouldn't a console.log or console.info effectively corrupt your communication stream, if it occurs during any part of the server execution?

Yes, this is discussed here: https://modelcontextprotocol.io/docs/tools/debugging#server-side-logging

Nedomas · 2025-01-08T00:36:09Z

Even though the fix here is for stdio specifically, I’m wondering isn’t the same leak happening in the SSE transport too?

I can’t seem to be getting rid of the active connection even with doing this:

constclient=newClient({name: 'superinterface-mcp-client',version: '1.0.0',},{capabilities: {}})consttransport=ewSSEClientTransport(newURL('some-url'))awaitclient.connect(transport)awaitclient.close()awaittransport.close()

But might be an unrelated bug?

jerome3o-anthropic · 2025-01-08T18:07:24Z

seem to be getting rid of the active connection even with doing

That's odd - likely a separate issue. Going to merge this now and follow up on the SSE bug later

…issue-114 fix(server) Avoid modifying path resolution for executables on MacOS

jerome3o-anthropic · 2025-01-03T16:32:00Z

Motivation and Context

#113

When closing a StdioServerTransport, the process would hang indefinitely due to unclosed stdin/stdout streams and lingering event listeners. This was particularly noticeable in server implementations that needed to shut down cleanly. The fix ensures proper cleanup of all resources, preventing process hangs.

How Has This Been Tested?

Added unit test in src/server/stdio.test.ts verifying proper cleanup of stream resources and event listeners
Added integration test in src/integration-tests/process-cleanup.test.ts that confirms the process exits cleanly after transport closure
Manually verified fix using a minimal reproduction case that previously demonstrated the hang

Breaking Changes

None. This is a bug fix that maintains the existing API contract.

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

The fix involves three key changes in StdioServerTransport.close():

Removing all event listeners, including 'drain' listeners that were previously overlooked
Explicitly destroying both stdin and stdout streams
Maintaining existing cleanup of the read buffer and onclose callback

The integration test was added to prevent regression, as unit tests alone didn't catch this issue due to the asynchronous nature of Node.js stream cleanup.

jspahrsummers

Thanks for working on this! Some blocking discussion, but this is definitely important

jspahrsummers · 2025-01-03T20:07:15Z

+ if (remainingDataListeners === 0) {
+ // Only pause stdin if we were the only listener
+ // This prevents interfering with other parts of the application that might be using stdin
+ this._stdin.pause();


I'm a bit surprised that this has any effect on termination, but then reading about Node.js readable streams, I'm like 😵
It feels a bit janky to do things this way, but I don't really have a better idea…

Per top-level PR comment, I don't see how both MCP and anything else can co-exist on the same stdin stream.

Certainly not while messages are streaming in. The only question is whether there'd ever be a reason for a server implementation to want to read from stdin after terminating the normal MCP stuff. Probably not? But it's always hard to anticipate how people will use a library.

davidfiala · 2025-01-03T23:38:37Z

I believe that since you are communicating a specific protocol over stdin/stdout, then it does assume you have permanently taken over both STDIO FDs.

This has interesting implications that I've had to work around. Specifically, wouldn't a console.log or console.info effectively corrupt your communication stream, if it occurs during any part of the server execution?

Here was an approximate workaround that I put at the top of servers to route the most common occurences to stderr instead:

// make console.log and console.info go to STDERR because MCP is going to take over STDOUTconstonlyStderr=newConsole({stdout: process.stderr,stderr: process.stderr});console.info=onlyStderr.info;console.log=onlyStderr.log;

In this case, I'd argue that yes it's fine/expected for you to end the stdin/stdout streams. It should be clearly documented that this behavior occurs IMHO. Likewise, perhaps you should hotpatch the console logs in your server.connect() on StdioServerTransport. The thing is, if the user happens to console.log BEFORE getting to your constructor, then there'd already be corruption-worthy output that the MCP client would be exposed to when it begins to read stdout. There's worse workarounds for that as well (ie, a shared sentinel to broadcast the true beginning of the MCP stream), but that is a breaking protocol change.

jspahrsummers · 2025-01-06T16:15:04Z

This has interesting implications that I've had to work around. Specifically, wouldn't a console.log or console.info effectively corrupt your communication stream, if it occurs during any part of the server execution?

Yes, this is discussed here: https://modelcontextprotocol.io/docs/tools/debugging#server-side-logging

Nedomas · 2025-01-08T00:36:09Z

Even though the fix here is for stdio specifically, I’m wondering isn’t the same leak happening in the SSE transport too?

I can’t seem to be getting rid of the active connection even with doing this:

constclient=newClient({name: 'superinterface-mcp-client',version: '1.0.0',},{capabilities: {}})consttransport=ewSSEClientTransport(newURL('some-url'))awaitclient.connect(transport)awaitclient.close()awaittransport.close()

But might be an unrelated bug?

jerome3o-anthropic · 2025-01-08T18:07:24Z

seem to be getting rid of the active connection even with doing

That's odd - likely a separate issue. Going to merge this now and follow up on the SSE bug later

…issue-114 fix(server) Avoid modifying path resolution for executables on MacOS

jerome3o-anthropic · 2025-01-03T16:32:00Z

Motivation and Context

#113

When closing a StdioServerTransport, the process would hang indefinitely due to unclosed stdin/stdout streams and lingering event listeners. This was particularly noticeable in server implementations that needed to shut down cleanly. The fix ensures proper cleanup of all resources, preventing process hangs.

How Has This Been Tested?

Added unit test in src/server/stdio.test.ts verifying proper cleanup of stream resources and event listeners
Added integration test in src/integration-tests/process-cleanup.test.ts that confirms the process exits cleanly after transport closure
Manually verified fix using a minimal reproduction case that previously demonstrated the hang

Breaking Changes

None. This is a bug fix that maintains the existing API contract.

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

The fix involves three key changes in StdioServerTransport.close():

Removing all event listeners, including 'drain' listeners that were previously overlooked
Explicitly destroying both stdin and stdout streams
Maintaining existing cleanup of the read buffer and onclose callback

The integration test was added to prevent regression, as unit tests alone didn't catch this issue due to the asynchronous nature of Node.js stream cleanup.

jspahrsummers

Thanks for working on this! Some blocking discussion, but this is definitely important

jspahrsummers · 2025-01-03T20:07:15Z

+ if (remainingDataListeners === 0) {
+ // Only pause stdin if we were the only listener
+ // This prevents interfering with other parts of the application that might be using stdin
+ this._stdin.pause();


I'm a bit surprised that this has any effect on termination, but then reading about Node.js readable streams, I'm like 😵
It feels a bit janky to do things this way, but I don't really have a better idea…

Per top-level PR comment, I don't see how both MCP and anything else can co-exist on the same stdin stream.

Certainly not while messages are streaming in. The only question is whether there'd ever be a reason for a server implementation to want to read from stdin after terminating the normal MCP stuff. Probably not? But it's always hard to anticipate how people will use a library.

davidfiala · 2025-01-03T23:38:37Z

I believe that since you are communicating a specific protocol over stdin/stdout, then it does assume you have permanently taken over both STDIO FDs.

This has interesting implications that I've had to work around. Specifically, wouldn't a console.log or console.info effectively corrupt your communication stream, if it occurs during any part of the server execution?

Here was an approximate workaround that I put at the top of servers to route the most common occurences to stderr instead:

// make console.log and console.info go to STDERR because MCP is going to take over STDOUTconstonlyStderr=newConsole({stdout: process.stderr,stderr: process.stderr});console.info=onlyStderr.info;console.log=onlyStderr.log;

In this case, I'd argue that yes it's fine/expected for you to end the stdin/stdout streams. It should be clearly documented that this behavior occurs IMHO. Likewise, perhaps you should hotpatch the console logs in your server.connect() on StdioServerTransport. The thing is, if the user happens to console.log BEFORE getting to your constructor, then there'd already be corruption-worthy output that the MCP client would be exposed to when it begins to read stdout. There's worse workarounds for that as well (ie, a shared sentinel to broadcast the true beginning of the MCP stream), but that is a breaking protocol change.

jspahrsummers · 2025-01-06T16:15:04Z

This has interesting implications that I've had to work around. Specifically, wouldn't a console.log or console.info effectively corrupt your communication stream, if it occurs during any part of the server execution?

Yes, this is discussed here: https://modelcontextprotocol.io/docs/tools/debugging#server-side-logging

Nedomas · 2025-01-08T00:36:09Z

Even though the fix here is for stdio specifically, I’m wondering isn’t the same leak happening in the SSE transport too?

I can’t seem to be getting rid of the active connection even with doing this:

constclient=newClient({name: 'superinterface-mcp-client',version: '1.0.0',},{capabilities: {}})consttransport=ewSSEClientTransport(newURL('some-url'))awaitclient.connect(transport)awaitclient.close()awaittransport.close()

But might be an unrelated bug?

jerome3o-anthropic · 2025-01-08T18:07:24Z

seem to be getting rid of the active connection even with doing

That's odd - likely a separate issue. Going to merge this now and follow up on the SSE bug later

…issue-114 fix(server) Avoid modifying path resolution for executables on MacOS

jerome3o-anthropic · 2025-01-03T16:32:00Z

Motivation and Context

#113

When closing a StdioServerTransport, the process would hang indefinitely due to unclosed stdin/stdout streams and lingering event listeners. This was particularly noticeable in server implementations that needed to shut down cleanly. The fix ensures proper cleanup of all resources, preventing process hangs.

How Has This Been Tested?

Added unit test in src/server/stdio.test.ts verifying proper cleanup of stream resources and event listeners
Added integration test in src/integration-tests/process-cleanup.test.ts that confirms the process exits cleanly after transport closure
Manually verified fix using a minimal reproduction case that previously demonstrated the hang

Breaking Changes

None. This is a bug fix that maintains the existing API contract.

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

The fix involves three key changes in StdioServerTransport.close():

Removing all event listeners, including 'drain' listeners that were previously overlooked
Explicitly destroying both stdin and stdout streams
Maintaining existing cleanup of the read buffer and onclose callback

The integration test was added to prevent regression, as unit tests alone didn't catch this issue due to the asynchronous nature of Node.js stream cleanup.

jspahrsummers

Thanks for working on this! Some blocking discussion, but this is definitely important

jspahrsummers · 2025-01-03T20:07:15Z

+ if (remainingDataListeners === 0) {
+ // Only pause stdin if we were the only listener
+ // This prevents interfering with other parts of the application that might be using stdin
+ this._stdin.pause();


I'm a bit surprised that this has any effect on termination, but then reading about Node.js readable streams, I'm like 😵
It feels a bit janky to do things this way, but I don't really have a better idea…

Per top-level PR comment, I don't see how both MCP and anything else can co-exist on the same stdin stream.

Certainly not while messages are streaming in. The only question is whether there'd ever be a reason for a server implementation to want to read from stdin after terminating the normal MCP stuff. Probably not? But it's always hard to anticipate how people will use a library.

davidfiala · 2025-01-03T23:38:37Z

I believe that since you are communicating a specific protocol over stdin/stdout, then it does assume you have permanently taken over both STDIO FDs.

This has interesting implications that I've had to work around. Specifically, wouldn't a console.log or console.info effectively corrupt your communication stream, if it occurs during any part of the server execution?

Here was an approximate workaround that I put at the top of servers to route the most common occurences to stderr instead:

// make console.log and console.info go to STDERR because MCP is going to take over STDOUTconstonlyStderr=newConsole({stdout: process.stderr,stderr: process.stderr});console.info=onlyStderr.info;console.log=onlyStderr.log;

In this case, I'd argue that yes it's fine/expected for you to end the stdin/stdout streams. It should be clearly documented that this behavior occurs IMHO. Likewise, perhaps you should hotpatch the console logs in your server.connect() on StdioServerTransport. The thing is, if the user happens to console.log BEFORE getting to your constructor, then there'd already be corruption-worthy output that the MCP client would be exposed to when it begins to read stdout. There's worse workarounds for that as well (ie, a shared sentinel to broadcast the true beginning of the MCP stream), but that is a breaking protocol change.

jspahrsummers · 2025-01-06T16:15:04Z

This has interesting implications that I've had to work around. Specifically, wouldn't a console.log or console.info effectively corrupt your communication stream, if it occurs during any part of the server execution?

Yes, this is discussed here: https://modelcontextprotocol.io/docs/tools/debugging#server-side-logging

Nedomas · 2025-01-08T00:36:09Z

Even though the fix here is for stdio specifically, I’m wondering isn’t the same leak happening in the SSE transport too?

I can’t seem to be getting rid of the active connection even with doing this:

constclient=newClient({name: 'superinterface-mcp-client',version: '1.0.0',},{capabilities: {}})consttransport=ewSSEClientTransport(newURL('some-url'))awaitclient.connect(transport)awaitclient.close()awaittransport.close()

But might be an unrelated bug?

jerome3o-anthropic · 2025-01-08T18:07:24Z

seem to be getting rid of the active connection even with doing

That's odd - likely a separate issue. Going to merge this now and follow up on the SSE bug later

…issue-114 fix(server) Avoid modifying path resolution for executables on MacOS

jerome3o-anthropic · 2025-01-03T16:32:00Z

Motivation and Context

#113

When closing a StdioServerTransport, the process would hang indefinitely due to unclosed stdin/stdout streams and lingering event listeners. This was particularly noticeable in server implementations that needed to shut down cleanly. The fix ensures proper cleanup of all resources, preventing process hangs.

How Has This Been Tested?

Added unit test in src/server/stdio.test.ts verifying proper cleanup of stream resources and event listeners
Added integration test in src/integration-tests/process-cleanup.test.ts that confirms the process exits cleanly after transport closure
Manually verified fix using a minimal reproduction case that previously demonstrated the hang

Breaking Changes

None. This is a bug fix that maintains the existing API contract.

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

The fix involves three key changes in StdioServerTransport.close():

Removing all event listeners, including 'drain' listeners that were previously overlooked
Explicitly destroying both stdin and stdout streams
Maintaining existing cleanup of the read buffer and onclose callback

The integration test was added to prevent regression, as unit tests alone didn't catch this issue due to the asynchronous nature of Node.js stream cleanup.

jspahrsummers

Thanks for working on this! Some blocking discussion, but this is definitely important

jspahrsummers · 2025-01-03T20:07:15Z

+ if (remainingDataListeners === 0) {
+ // Only pause stdin if we were the only listener
+ // This prevents interfering with other parts of the application that might be using stdin
+ this._stdin.pause();


I'm a bit surprised that this has any effect on termination, but then reading about Node.js readable streams, I'm like 😵
It feels a bit janky to do things this way, but I don't really have a better idea…

Per top-level PR comment, I don't see how both MCP and anything else can co-exist on the same stdin stream.

Certainly not while messages are streaming in. The only question is whether there'd ever be a reason for a server implementation to want to read from stdin after terminating the normal MCP stuff. Probably not? But it's always hard to anticipate how people will use a library.

davidfiala · 2025-01-03T23:38:37Z

I believe that since you are communicating a specific protocol over stdin/stdout, then it does assume you have permanently taken over both STDIO FDs.

This has interesting implications that I've had to work around. Specifically, wouldn't a console.log or console.info effectively corrupt your communication stream, if it occurs during any part of the server execution?

Here was an approximate workaround that I put at the top of servers to route the most common occurences to stderr instead:

// make console.log and console.info go to STDERR because MCP is going to take over STDOUTconstonlyStderr=newConsole({stdout: process.stderr,stderr: process.stderr});console.info=onlyStderr.info;console.log=onlyStderr.log;

In this case, I'd argue that yes it's fine/expected for you to end the stdin/stdout streams. It should be clearly documented that this behavior occurs IMHO. Likewise, perhaps you should hotpatch the console logs in your server.connect() on StdioServerTransport. The thing is, if the user happens to console.log BEFORE getting to your constructor, then there'd already be corruption-worthy output that the MCP client would be exposed to when it begins to read stdout. There's worse workarounds for that as well (ie, a shared sentinel to broadcast the true beginning of the MCP stream), but that is a breaking protocol change.

jspahrsummers · 2025-01-06T16:15:04Z

This has interesting implications that I've had to work around. Specifically, wouldn't a console.log or console.info effectively corrupt your communication stream, if it occurs during any part of the server execution?

Yes, this is discussed here: https://modelcontextprotocol.io/docs/tools/debugging#server-side-logging

Nedomas · 2025-01-08T00:36:09Z

Even though the fix here is for stdio specifically, I’m wondering isn’t the same leak happening in the SSE transport too?

I can’t seem to be getting rid of the active connection even with doing this:

constclient=newClient({name: 'superinterface-mcp-client',version: '1.0.0',},{capabilities: {}})consttransport=ewSSEClientTransport(newURL('some-url'))awaitclient.connect(transport)awaitclient.close()awaittransport.close()

But might be an unrelated bug?

jerome3o-anthropic · 2025-01-08T18:07:24Z

seem to be getting rid of the active connection even with doing

That's odd - likely a separate issue. Going to merge this now and follow up on the SSE bug later

…issue-114 fix(server) Avoid modifying path resolution for executables on MacOS

jerome3o-anthropic · 2025-01-03T16:32:00Z

Motivation and Context

#113

When closing a StdioServerTransport, the process would hang indefinitely due to unclosed stdin/stdout streams and lingering event listeners. This was particularly noticeable in server implementations that needed to shut down cleanly. The fix ensures proper cleanup of all resources, preventing process hangs.

How Has This Been Tested?

Added unit test in src/server/stdio.test.ts verifying proper cleanup of stream resources and event listeners
Added integration test in src/integration-tests/process-cleanup.test.ts that confirms the process exits cleanly after transport closure
Manually verified fix using a minimal reproduction case that previously demonstrated the hang

Breaking Changes

None. This is a bug fix that maintains the existing API contract.

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

The fix involves three key changes in StdioServerTransport.close():

Removing all event listeners, including 'drain' listeners that were previously overlooked
Explicitly destroying both stdin and stdout streams
Maintaining existing cleanup of the read buffer and onclose callback

The integration test was added to prevent regression, as unit tests alone didn't catch this issue due to the asynchronous nature of Node.js stream cleanup.

jspahrsummers

Thanks for working on this! Some blocking discussion, but this is definitely important

jspahrsummers · 2025-01-03T20:07:15Z

+ if (remainingDataListeners === 0) {
+ // Only pause stdin if we were the only listener
+ // This prevents interfering with other parts of the application that might be using stdin
+ this._stdin.pause();


I'm a bit surprised that this has any effect on termination, but then reading about Node.js readable streams, I'm like 😵
It feels a bit janky to do things this way, but I don't really have a better idea…

Per top-level PR comment, I don't see how both MCP and anything else can co-exist on the same stdin stream.

Certainly not while messages are streaming in. The only question is whether there'd ever be a reason for a server implementation to want to read from stdin after terminating the normal MCP stuff. Probably not? But it's always hard to anticipate how people will use a library.

davidfiala · 2025-01-03T23:38:37Z

I believe that since you are communicating a specific protocol over stdin/stdout, then it does assume you have permanently taken over both STDIO FDs.

This has interesting implications that I've had to work around. Specifically, wouldn't a console.log or console.info effectively corrupt your communication stream, if it occurs during any part of the server execution?

Here was an approximate workaround that I put at the top of servers to route the most common occurences to stderr instead:

// make console.log and console.info go to STDERR because MCP is going to take over STDOUTconstonlyStderr=newConsole({stdout: process.stderr,stderr: process.stderr});console.info=onlyStderr.info;console.log=onlyStderr.log;

In this case, I'd argue that yes it's fine/expected for you to end the stdin/stdout streams. It should be clearly documented that this behavior occurs IMHO. Likewise, perhaps you should hotpatch the console logs in your server.connect() on StdioServerTransport. The thing is, if the user happens to console.log BEFORE getting to your constructor, then there'd already be corruption-worthy output that the MCP client would be exposed to when it begins to read stdout. There's worse workarounds for that as well (ie, a shared sentinel to broadcast the true beginning of the MCP stream), but that is a breaking protocol change.

jspahrsummers · 2025-01-06T16:15:04Z

This has interesting implications that I've had to work around. Specifically, wouldn't a console.log or console.info effectively corrupt your communication stream, if it occurs during any part of the server execution?

Yes, this is discussed here: https://modelcontextprotocol.io/docs/tools/debugging#server-side-logging

Nedomas · 2025-01-08T00:36:09Z

Even though the fix here is for stdio specifically, I’m wondering isn’t the same leak happening in the SSE transport too?

I can’t seem to be getting rid of the active connection even with doing this:

constclient=newClient({name: 'superinterface-mcp-client',version: '1.0.0',},{capabilities: {}})consttransport=ewSSEClientTransport(newURL('some-url'))awaitclient.connect(transport)awaitclient.close()awaittransport.close()

But might be an unrelated bug?

jerome3o-anthropic · 2025-01-08T18:07:24Z

seem to be getting rid of the active connection even with doing

That's odd - likely a separate issue. Going to merge this now and follow up on the SSE bug later

…issue-114 fix(server) Avoid modifying path resolution for executables on MacOS

jerome3o-anthropic · 2025-01-03T16:32:00Z

Motivation and Context

#113

When closing a StdioServerTransport, the process would hang indefinitely due to unclosed stdin/stdout streams and lingering event listeners. This was particularly noticeable in server implementations that needed to shut down cleanly. The fix ensures proper cleanup of all resources, preventing process hangs.

How Has This Been Tested?

Added unit test in src/server/stdio.test.ts verifying proper cleanup of stream resources and event listeners
Added integration test in src/integration-tests/process-cleanup.test.ts that confirms the process exits cleanly after transport closure
Manually verified fix using a minimal reproduction case that previously demonstrated the hang

Breaking Changes

None. This is a bug fix that maintains the existing API contract.

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

The fix involves three key changes in StdioServerTransport.close():

Removing all event listeners, including 'drain' listeners that were previously overlooked
Explicitly destroying both stdin and stdout streams
Maintaining existing cleanup of the read buffer and onclose callback

The integration test was added to prevent regression, as unit tests alone didn't catch this issue due to the asynchronous nature of Node.js stream cleanup.

jspahrsummers

Thanks for working on this! Some blocking discussion, but this is definitely important

jspahrsummers · 2025-01-03T20:07:15Z

+ if (remainingDataListeners === 0) {
+ // Only pause stdin if we were the only listener
+ // This prevents interfering with other parts of the application that might be using stdin
+ this._stdin.pause();


I'm a bit surprised that this has any effect on termination, but then reading about Node.js readable streams, I'm like 😵
It feels a bit janky to do things this way, but I don't really have a better idea…

Per top-level PR comment, I don't see how both MCP and anything else can co-exist on the same stdin stream.

Certainly not while messages are streaming in. The only question is whether there'd ever be a reason for a server implementation to want to read from stdin after terminating the normal MCP stuff. Probably not? But it's always hard to anticipate how people will use a library.

davidfiala · 2025-01-03T23:38:37Z

I believe that since you are communicating a specific protocol over stdin/stdout, then it does assume you have permanently taken over both STDIO FDs.

This has interesting implications that I've had to work around. Specifically, wouldn't a console.log or console.info effectively corrupt your communication stream, if it occurs during any part of the server execution?

Here was an approximate workaround that I put at the top of servers to route the most common occurences to stderr instead:

// make console.log and console.info go to STDERR because MCP is going to take over STDOUTconstonlyStderr=newConsole({stdout: process.stderr,stderr: process.stderr});console.info=onlyStderr.info;console.log=onlyStderr.log;

In this case, I'd argue that yes it's fine/expected for you to end the stdin/stdout streams. It should be clearly documented that this behavior occurs IMHO. Likewise, perhaps you should hotpatch the console logs in your server.connect() on StdioServerTransport. The thing is, if the user happens to console.log BEFORE getting to your constructor, then there'd already be corruption-worthy output that the MCP client would be exposed to when it begins to read stdout. There's worse workarounds for that as well (ie, a shared sentinel to broadcast the true beginning of the MCP stream), but that is a breaking protocol change.

jspahrsummers · 2025-01-06T16:15:04Z

This has interesting implications that I've had to work around. Specifically, wouldn't a console.log or console.info effectively corrupt your communication stream, if it occurs during any part of the server execution?

Yes, this is discussed here: https://modelcontextprotocol.io/docs/tools/debugging#server-side-logging

Nedomas · 2025-01-08T00:36:09Z

Even though the fix here is for stdio specifically, I’m wondering isn’t the same leak happening in the SSE transport too?

I can’t seem to be getting rid of the active connection even with doing this:

constclient=newClient({name: 'superinterface-mcp-client',version: '1.0.0',},{capabilities: {}})consttransport=ewSSEClientTransport(newURL('some-url'))awaitclient.connect(transport)awaitclient.close()awaittransport.close()

But might be an unrelated bug?

jerome3o-anthropic · 2025-01-08T18:07:24Z

seem to be getting rid of the active connection even with doing

That's odd - likely a separate issue. Going to merge this now and follow up on the SSE bug later

…issue-114 fix(server) Avoid modifying path resolution for executables on MacOS

Added clean shutdown + tests
7c91349

jerome3o-anthropic requested a review from jspahrsummers January 3, 2025 16:32

jspahrsummers requested changes Jan 3, 2025
View reviewed changes

Comment threadsrc/server/stdio.ts Outdated
Comment threadsrc/server/stdio.ts Outdated

jspahrsummers linked an issue Jan 3, 2025 that may be closed by this pull request
Unable to cleanly shutdown/close MCP Server #113
Closed

jerome3o-anthropic added 3 commits January 3, 2025 18:47

Don't remove drain listeners
6ebf67e

Pause stdin if we were the only listeners to the data events
8a07747

Removed unneeded test
a2157dc

jerome3o-anthropic requested a review from jspahrsummers January 3, 2025 19:11

jspahrsummers approved these changes Jan 3, 2025
View reviewed changes

jerome3o-anthropic merged commit e72ff8d into mainJan 8, 2025

jerome3o-anthropic deleted the jerome/fix/clean-stdio-shutdown branch January 8, 2025 18:07

Pizzaface pushed a commit to RewstApp/mcp-inspector that referenced this pull request May 2, 2025
Merge pull requestmodelcontextprotocol#115from jacksteamdev/bugfix/…
d4df126
…issue-114 fix(server) Avoid modifying path resolution for executables on MacOS

Added clean shutdown + tests
7c91349

jerome3o-anthropic requested a review from jspahrsummers January 3, 2025 16:32

jspahrsummers requested changes Jan 3, 2025
View reviewed changes

Comment threadsrc/server/stdio.ts Outdated
Comment threadsrc/server/stdio.ts Outdated

jspahrsummers linked an issue Jan 3, 2025 that may be closed by this pull request
Unable to cleanly shutdown/close MCP Server #113
Closed

jerome3o-anthropic added 3 commits January 3, 2025 18:47

Don't remove drain listeners
6ebf67e

Pause stdin if we were the only listeners to the data events
8a07747

Removed unneeded test
a2157dc

jerome3o-anthropic requested a review from jspahrsummers January 3, 2025 19:11

jspahrsummers approved these changes Jan 3, 2025
View reviewed changes

jerome3o-anthropic merged commit e72ff8d into mainJan 8, 2025

jerome3o-anthropic deleted the jerome/fix/clean-stdio-shutdown branch January 8, 2025 18:07

Pizzaface pushed a commit to RewstApp/mcp-inspector that referenced this pull request May 2, 2025
Merge pull requestmodelcontextprotocol#115from jacksteamdev/bugfix/…
d4df126
…issue-114 fix(server) Avoid modifying path resolution for executables on MacOS

Added clean shutdown + tests
7c91349

jerome3o-anthropic requested a review from jspahrsummers January 3, 2025 16:32

jspahrsummers requested changes Jan 3, 2025
View reviewed changes

Comment threadsrc/server/stdio.ts Outdated
Comment threadsrc/server/stdio.ts Outdated

jspahrsummers linked an issue Jan 3, 2025 that may be closed by this pull request
Unable to cleanly shutdown/close MCP Server #113
Closed

jerome3o-anthropic added 3 commits January 3, 2025 18:47

Don't remove drain listeners
6ebf67e

Pause stdin if we were the only listeners to the data events
8a07747

Removed unneeded test
a2157dc

jerome3o-anthropic requested a review from jspahrsummers January 3, 2025 19:11

jspahrsummers approved these changes Jan 3, 2025
View reviewed changes

jerome3o-anthropic merged commit e72ff8d into mainJan 8, 2025

jerome3o-anthropic deleted the jerome/fix/clean-stdio-shutdown branch January 8, 2025 18:07

Pizzaface pushed a commit to RewstApp/mcp-inspector that referenced this pull request May 2, 2025
Merge pull requestmodelcontextprotocol#115from jacksteamdev/bugfix/…
d4df126
…issue-114 fix(server) Avoid modifying path resolution for executables on MacOS

Added clean shutdown + tests
7c91349

jerome3o-anthropic requested a review from jspahrsummers January 3, 2025 16:32

jspahrsummers requested changes Jan 3, 2025
View reviewed changes

Comment threadsrc/server/stdio.ts Outdated
Comment threadsrc/server/stdio.ts Outdated

jspahrsummers linked an issue Jan 3, 2025 that may be closed by this pull request
Unable to cleanly shutdown/close MCP Server #113
Closed

jerome3o-anthropic added 3 commits January 3, 2025 18:47

Don't remove drain listeners
6ebf67e

Pause stdin if we were the only listeners to the data events
8a07747

Removed unneeded test
a2157dc

jerome3o-anthropic requested a review from jspahrsummers January 3, 2025 19:11

jspahrsummers approved these changes Jan 3, 2025
View reviewed changes

jerome3o-anthropic merged commit e72ff8d into mainJan 8, 2025

jerome3o-anthropic deleted the jerome/fix/clean-stdio-shutdown branch January 8, 2025 18:07

Pizzaface pushed a commit to RewstApp/mcp-inspector that referenced this pull request May 2, 2025
Merge pull requestmodelcontextprotocol#115from jacksteamdev/bugfix/…
d4df126
…issue-114 fix(server) Avoid modifying path resolution for executables on MacOS

Added clean shutdown + tests
7c91349

jerome3o-anthropic requested a review from jspahrsummers January 3, 2025 16:32

jspahrsummers requested changes Jan 3, 2025
View reviewed changes

Comment threadsrc/server/stdio.ts Outdated
Comment threadsrc/server/stdio.ts Outdated

jspahrsummers linked an issue Jan 3, 2025 that may be closed by this pull request
Unable to cleanly shutdown/close MCP Server #113
Closed

jerome3o-anthropic added 3 commits January 3, 2025 18:47

Don't remove drain listeners
6ebf67e

Pause stdin if we were the only listeners to the data events
8a07747

Removed unneeded test
a2157dc

jerome3o-anthropic requested a review from jspahrsummers January 3, 2025 19:11

jspahrsummers approved these changes Jan 3, 2025
View reviewed changes

jerome3o-anthropic merged commit e72ff8d into mainJan 8, 2025

jerome3o-anthropic deleted the jerome/fix/clean-stdio-shutdown branch January 8, 2025 18:07

Pizzaface pushed a commit to RewstApp/mcp-inspector that referenced this pull request May 2, 2025
Merge pull requestmodelcontextprotocol#115from jacksteamdev/bugfix/…
d4df126
…issue-114 fix(server) Avoid modifying path resolution for executables on MacOS

Added clean shutdown + tests
7c91349

jerome3o-anthropic requested a review from jspahrsummers January 3, 2025 16:32

jspahrsummers requested changes Jan 3, 2025
View reviewed changes

Comment threadsrc/server/stdio.ts Outdated
Comment threadsrc/server/stdio.ts Outdated

jspahrsummers linked an issue Jan 3, 2025 that may be closed by this pull request
Unable to cleanly shutdown/close MCP Server #113
Closed

jerome3o-anthropic added 3 commits January 3, 2025 18:47

Don't remove drain listeners
6ebf67e

Pause stdin if we were the only listeners to the data events
8a07747

Removed unneeded test
a2157dc

jerome3o-anthropic requested a review from jspahrsummers January 3, 2025 19:11

jspahrsummers approved these changes Jan 3, 2025
View reviewed changes

jerome3o-anthropic merged commit e72ff8d into mainJan 8, 2025

jerome3o-anthropic deleted the jerome/fix/clean-stdio-shutdown branch January 8, 2025 18:07

Pizzaface pushed a commit to RewstApp/mcp-inspector that referenced this pull request May 2, 2025
Merge pull requestmodelcontextprotocol#115from jacksteamdev/bugfix/…
d4df126
…issue-114 fix(server) Avoid modifying path resolution for executables on MacOS

Added clean shutdown + tests
7c91349

jerome3o-anthropic requested a review from jspahrsummers January 3, 2025 16:32

jspahrsummers requested changes Jan 3, 2025
View reviewed changes

Comment threadsrc/server/stdio.ts Outdated
Comment threadsrc/server/stdio.ts Outdated

jspahrsummers linked an issue Jan 3, 2025 that may be closed by this pull request
Unable to cleanly shutdown/close MCP Server #113
Closed

jerome3o-anthropic added 3 commits January 3, 2025 18:47

Don't remove drain listeners
6ebf67e

Pause stdin if we were the only listeners to the data events
8a07747

Removed unneeded test
a2157dc

jerome3o-anthropic requested a review from jspahrsummers January 3, 2025 19:11

jspahrsummers approved these changes Jan 3, 2025
View reviewed changes

jerome3o-anthropic merged commit e72ff8d into mainJan 8, 2025

jerome3o-anthropic deleted the jerome/fix/clean-stdio-shutdown branch January 8, 2025 18:07

Pizzaface pushed a commit to RewstApp/mcp-inspector that referenced this pull request May 2, 2025
Merge pull requestmodelcontextprotocol#115from jacksteamdev/bugfix/…
d4df126
…issue-114 fix(server) Avoid modifying path resolution for executables on MacOS

Added clean shutdown + tests
7c91349

jerome3o-anthropic requested a review from jspahrsummers January 3, 2025 16:32

jspahrsummers requested changes Jan 3, 2025
View reviewed changes

Comment threadsrc/server/stdio.ts Outdated
Comment threadsrc/server/stdio.ts Outdated

jspahrsummers linked an issue Jan 3, 2025 that may be closed by this pull request
Unable to cleanly shutdown/close MCP Server #113
Closed

jerome3o-anthropic added 3 commits January 3, 2025 18:47

Don't remove drain listeners
6ebf67e

Pause stdin if we were the only listeners to the data events
8a07747

Removed unneeded test
a2157dc

jerome3o-anthropic requested a review from jspahrsummers January 3, 2025 19:11

jspahrsummers approved these changes Jan 3, 2025
View reviewed changes

jerome3o-anthropic merged commit e72ff8d into mainJan 8, 2025

jerome3o-anthropic deleted the jerome/fix/clean-stdio-shutdown branch January 8, 2025 18:07

Pizzaface pushed a commit to RewstApp/mcp-inspector that referenced this pull request May 2, 2025
Merge pull requestmodelcontextprotocol#115from jacksteamdev/bugfix/…
d4df126
…issue-114 fix(server) Avoid modifying path resolution for executables on MacOS

Uh oh!

Conversation

jerome3o-anthropic commented Jan 3, 2025

Motivation and Context

How Has This Been Tested?

Breaking Changes

Types of changes

Checklist

Additional context

Uh oh!

jspahrsummers left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidfiala commented Jan 3, 2025

Uh oh!

jspahrsummers commented Jan 6, 2025

Uh oh!

Nedomas commented Jan 8, 2025

Uh oh!

jerome3o-anthropic commented Jan 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

jerome3o-anthropic commented Jan 3, 2025

Motivation and Context

How Has This Been Tested?

Breaking Changes

Types of changes

Checklist

Additional context

Uh oh!

jspahrsummers left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidfiala commented Jan 3, 2025

Uh oh!

jspahrsummers commented Jan 6, 2025

Uh oh!

Nedomas commented Jan 8, 2025

Uh oh!

jerome3o-anthropic commented Jan 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

jerome3o-anthropic commented Jan 3, 2025

Motivation and Context

How Has This Been Tested?

Breaking Changes