fix: abort in-flight request handlers on connection close#1735
fix: abort in-flight request handlers on connection close#1735felixweinberger wants to merge 9 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: 6dd5629 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 |
There was a problem hiding this comment.
All previous feedback addressed — LGTM.
Extended reasoning...
Overview
This PR adds abort signal firing for in-flight request handlers when the transport connection closes, cleans up timeout handles and request resolvers on close, and makes InMemoryTransport.close() idempotent. Files modified: protocol.ts, taskManager.ts, inMemory.ts, plus corresponding tests.
Security risks
No security risks. The changes improve resource cleanup on disconnect, which is purely defensive.
Level of scrutiny
Moderate scrutiny was warranted due to the changes touching Protocol lifecycle cleanup (_onclose). The core logic is straightforward: snapshot-and-clear maps, then iterate the snapshots. The author addressed all three issues raised in previous reviews: the critical _requestResolvers misplacement (moved to TaskManager.onClose()), the _timeoutInfo leak (now cleared with clearTimeout), and the try/finally wrapping of this.onclose?.() for defensive cleanup.
Other factors
The PR includes comprehensive test coverage: abort signal firing on close, single-fire/double-close/concurrent-close idempotency for InMemoryTransport, and a test for onclose firing even when the peer throws. All 444 core tests pass per the author. The changes follow existing patterns in the codebase and have no breaking changes.
bhosmer-ant
left a comment
bhosmer-ant
left a comment
There was a problem hiding this comment.
Nice fix — addresses the core issue and all prior review feedback cleanly. Two things worth tightening up before merge (both small, both keep the PR's safety guarantees consistent):
bhosmer-ant
left a comment
bhosmer-ant
left a comment
There was a problem hiding this comment.
Upgrading my earlier comment to request-changes so you can re-request when ready — the two inline findings there (missing try/finally in connect()'s onclose wrapper, and the map-swap stale-reference race with _onrequest's .finally() cleanup) are both small fixes.
|
Hi, This also is somewhat related to #1283 which I was looking at, where |
…ler delete - Wrap preserved transport onclose in try/finally so _onclose() runs even if the transport-level handler throws - Add identity check before deleting from _requestHandlerAbortControllers in the .finally() cleanup to avoid deleting a reconnected request's controller when IDs collide across close/reconnect
felixweinberger
dismissed
bhosmer-ant’s stale review
Addressed requested changes
The override prevented the base class _requestResolvers cleanup from running. Base TaskManager.onClose just clears two maps, which is cheap and correct for the null variant too.
bhosmer-ant
left a comment
bhosmer-ant
left a comment
There was a problem hiding this comment.
Both findings addressed — the try/finally in connect() and the identity check in the .finally() cleanup are exactly right. LGTM.
Two observations for possible follow-up (non-blocking, both about interactions with pre-existing code):
Task result loss on close: The new abort-on-close can race with task result persistence. The pre-existing abort check in _onrequest (~lines 621-624, 639-642) does if (signal.aborted) return; before routeResponse(). Previously the abort only came from explicit notifications/cancelled; now it also comes from close. If a handler completes and produces a task result, but close fires before routeResponse runs, the result is dropped — which matters for task resumability across reconnections. Narrow window and tasks are experimental, so not urgent, but the fix would be to check signal.reason and still call routeResponse (skip only the capturedTransport.send) when the reason is ConnectionClosed.
_onclose itself isn't idempotent: The _closed flag on InMemoryTransport is the right fix for that transport, but Protocol._onclose() has no guard of its own. If some other transport double-fires onclose, this.onclose?.() runs twice. The map swaps make the second run mostly a no-op, but the user callback isn't. Belt-and-suspenders would be an early-return if _transport is already undefined.
3 participants
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.