-
Notifications
You must be signed in to change notification settings - Fork 1.8k
fix: prevent unhandled rejections on close and detect stdin EOF #1853
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.
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
Closed
EtanHey
wants to merge
3
commits into
modelcontextprotocol:main
from
EtanHey:fix/connection-closed-unhandled-rejection
Closed
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| --- | ||
| '@modelcontextprotocol/core': patch | ||
| '@modelcontextprotocol/server': patch | ||
| --- | ||
|
|
||
| Fix unhandled promise rejections on transport close and detect stdin EOF in StdioServerTransport. Pending request promises are now rejected asynchronously via microtask deferral, and the server transport listens for stdin `end` events to trigger a clean shutdown when the client process exits. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 The test pre-attaches
requestPromise.catch()synchronously before callingtransport.close(), so the rejection handler is already in place when the rejection fires regardless of whether it fires synchronously (old code) or via deferred microtask (new code). This means the test passes equally well with the old synchronous rejection code and does not actually validate that the microtask deferral prevents unhandled rejections. Consider restructuring the test to close the transport first and then observe that a request made immediately afterward does not produce an unhandled rejection without an explicit.catch().Extended reasoning...
What the test does vs. what it should validate
The test is intended to verify that deferring
handler(error)calls to the next microtask (viaPromise.resolve().then()) prevents unhandled promise rejections. The race condition the PR fixes is: a transport closes and synchronously rejects all pending promises before the caller has had a chance to attach a.catch()handler. On Node.js 24 with--unhandled-rejections=throw, such an unhandled rejection kills the process.Why the test gives false assurance
The test attaches
requestPromise.catch(() => {})synchronously and immediately after creating the request promise — well beforetransport.close()is called. Because.catch()is already registered, no unhandled rejection can occur regardless of when the underlyingreject()call fires:transport.close()→_onclose()→handler(error)→reject(error)fires synchronously. But.catch()was attached two lines earlier, so Node.js sees the rejection as handled. ✓ test passes.reject(error)fires in the next microtask..catch()was still attached before close, so the rejection is still handled. ✓ test passes.Both implementations pass the test with identical results; the deferral change is entirely unobservable by this test.
The actual race condition is not exercised
The true race occurs when: (1) a request promise is created, (2) the transport closes synchronously in the same JS tick, and (3) the caller hasn't yet attached
.catch()— e.g., because the close happens inside the same synchronous block before the caller returns to user code. The test eliminates this scenario by pre-attaching the handler.Impact
The production code change is correct and the fix is real. However, the test provides false confidence that the fix is validated. If someone reverts the deferral change (or introduces a regression), this test would still pass, making it ineffective as a regression guard.
How to fix the test
A correct test would:
transport.close()without any pending request.protocol.request()to get a promise in a state where the transport is already closed..catch()before verifying the unhandledRejection listener count stays at zero.Alternatively: capture the promise, do NOT attach
.catch(), flush microtasks, then attach.catch()and assert no unhandledRejection fired. This directly reproduces the race because the rejection fires (in the next microtask) while no handler is attached.