fix: abort in-flight request handlers on connection close#1735
fix: abort in-flight request handlers on connection close#1735felixweinberger wants to merge 7 commits intomainfrom
Conversation
Previously, request handlers would continue running after the transport disconnected, wasting resources and preventing cleanup of long-running operations. Protocol._onclose() now aborts all active request handler AbortControllers with a ConnectionClosed error. Also fixes InMemoryTransport.close() firing onclose twice on the initiating side due to peer recursion. Fixes #611 Co-authored-by: Aljosa Asanovic <aljosa.a@gmail.com>
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, well-tested fix that connects existing abort controller infrastructure to the connection close path.
Extended reasoning...
Overview
This PR makes two small, focused changes: (1) in Protocol._onclose(), it aborts all in-flight request handler abort controllers when the connection closes, and (2) it makes InMemoryTransport.close() idempotent with a _closed guard. Both changes are accompanied by new tests.
Security risks
No security concerns. The changes are purely about resource cleanup on disconnect — no auth, crypto, or permission logic is involved.
Level of scrutiny
The protocol.ts change adds 6 lines that follow the exact same save-replace-iterate pattern already used for _responseHandlers cleanup 3 lines above. The InMemoryTransport change is a standard idempotency guard (3 lines). Both are mechanical and low-risk. The two bug reports found are pre-existing issues (timeout handle leak in _onclose) and a very low-impact edge case in a test utility (InMemoryTransport.close missing try-finally), neither of which are regressions from this PR.
Other factors
The abort controller infrastructure (_requestHandlerAbortControllers map, creation in _onrequest, cleanup in .finally(), cancellation via notifications/cancelled) already existed — this PR simply connects the same mechanism to the close event. Test coverage is good: the protocol test verifies the abort signal fires with ConnectionClosed, and three InMemoryTransport tests cover single-fire, double-close, and concurrent close scenarios.
… throws - Clear _timeoutInfo and _requestResolvers in _onclose() to prevent leaked setTimeout handles from keeping the event loop alive - Wrap peer close in try-finally so InMemoryTransport.onclose fires even if the peer's onclose callback throws
🦋 Changeset detectedLatest commit: 01193f8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@claude review |
There was a problem hiding this comment.
Previous feedback addressed, LGTM.
Extended reasoning...
Overview
This PR adds abort controller cleanup to Protocol._onclose() so in-flight request handlers are aborted on disconnect, and makes InMemoryTransport.close() idempotent with a _closed guard and try/finally for robustness. It also opportunistically cleans up _timeoutInfo and _requestResolvers on close. Five files are modified: the two source files, their corresponding test files, and a changeset.
Security risks
None. The changes are purely internal cleanup logic — no auth, crypto, or permission changes.
Level of scrutiny
This is a well-scoped bug fix that extends an existing cleanup pattern (clearing maps and aborting controllers in _onclose) to cover a previously-missed case. The abort controller infrastructure already existed for notifications/cancelled; this just connects the same mechanism to connection close. The InMemoryTransport changes are to a test utility. Both changes are low-risk.
Other factors
The author addressed both of my previous review comments (timeout cleanup and try/finally in InMemoryTransport) in commit 3b88756. The new test coverage is thorough: protocol-level abort signal test, and InMemoryTransport tests for single-fire, double-close idempotency, concurrent close, and error propagation. The remaining nit (try/finally around this.onclose?.() in _onclose()) is pre-existing and minor — it does not block approval.
Wrap the user-provided onclose callback in try/finally so response handler rejection and abort controller firing happen even if the callback throws. Consistent with the pattern in InMemoryTransport.
There was a problem hiding this comment.
All previous review feedback has been addressed — LGTM.
Extended reasoning...
Overview
This PR makes two targeted changes to @modelcontextprotocol/core:
Protocol._onclose()now aborts in-flight request handlerAbortControllers on connection close, clears_timeoutInfo(fixing a pre-existing timer leak), clears_requestResolvers, and wraps the useronclosecallback intry/finallyso cleanup always runs.InMemoryTransport.close()is made idempotent via a_closedflag and usestry/finallyto ensureonclosefires even if the peer throws.
Five files are touched: protocol.ts, inMemory.ts, their respective test files, and a changeset.
Security risks
None. The changes are internal cleanup logic with no auth, crypto, or permission implications.
Level of scrutiny
The core change — aborting request handler abort controllers on close — leverages an existing mechanism (the _requestHandlerAbortControllers map, already used for notifications/cancelled). The new code is ~15 lines of cleanup logic in _onclose() plus an idempotency guard in InMemoryTransport. Both are well-tested with 5 new test cases covering the abort signal, single-fire, double-close, concurrent close, and peer-throws scenarios.
Other factors
This PR went through three rounds of review from me. All feedback — try/finally in InMemoryTransport.close(), clearing _timeoutInfo/_requestResolvers in _onclose(), and try/finally around the user onclose callback — has been addressed in subsequent commits. Both prior inline comments are marked as resolved. The changes are non-breaking (handlers that ignore signal are unaffected) and the changeset is correctly configured as a patch release.
There was a problem hiding this comment.
LGTM — all previous review feedback addressed, cleanup logic is correct and well-tested.
Extended reasoning...
Overview
This PR touches two files in packages/core/src: protocol.ts (the core Protocol class _onclose() method) and inMemory.ts (the InMemoryTransport.close() method), plus corresponding test files. The changes abort in-flight request handler AbortControllers on connection close, make InMemoryTransport.close() idempotent with a _closed guard, and clean up previously leaked _timeoutInfo and _requestResolvers maps.
Security risks
None. The changes are internal cleanup logic for connection lifecycle management. No auth, crypto, permissions, or external-facing code is modified.
Level of scrutiny
This is core protocol infrastructure, which normally warrants careful review. However, all three rounds of review feedback (try-finally in InMemoryTransport, try-finally in Protocol._onclose, and timeout/resolver cleanup) have been addressed in follow-up commits. The changes follow established patterns already present in the codebase (abort controllers for notifications/cancelled, response handler cleanup in _onclose). The behavioral change is opt-in — only handlers that already listen to ctx.mcpReq.signal are affected.
Other factors
Five new tests cover the critical scenarios: single-fire onclose, double-close idempotency, concurrent close, peer-throws-still-fires-onclose, and abort-signal-on-connection-close. The changeset is included. All previous inline comments are resolved with corresponding code changes.
…ctor The TaskManager extraction moved _requestResolvers to the new class but the merge left a stale reference in Protocol._onclose. Relocate the cleanup to TaskManager.onClose alongside _taskProgressTokens.
|
@claude review |
1 participant
Aborts active request handlers when the transport connection closes, and makes
InMemoryTransport.close()idempotent.Salvaged from #833 by @alasano, ported to the v2 package structure with the Protocol tests that were requested in review.
Motivation and Context
When a client disconnects mid-request (network failure, timeout, crash), the server's
Protocol._onclose()cleans up response handlers but leaves in-flight request handlers running. Long-running operations (file uploads, external API calls, elicitation prompts) continue indefinitely, wasting resources and causing hangs.Separately,
InMemoryTransport.close()recurses through the peer and firesonclosetwice on the initiating side.Fixes #611. Supersedes #833.
How Has This Been Tested?
AbortSignalfires withConnectionClosedon transport closeBreaking Changes
None. Request handlers that ignore
ctx.mcpReq.signalare unaffected; handlers that respect it will now abort cleanly instead of running to completion after disconnect.Types of changes
Checklist
Additional context
The abort controllers infrastructure already existed for explicit
notifications/cancelledhandling; this connects the same mechanism to connection close events.