fix(core): stop reporting cancellation as an error - #998
Conversation
…Cancellable` to prevent suppressed cancellations.
There was a problem hiding this comment.
🟡 Changes recommended
The cancellation test does not compile because it passes a suspend function to a non-suspending helper.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates MCP transport and protocol handling so CancellationException is propagated without being logged or reported as an error.
Changes:
- Adds cancellation-aware exception handling and tests.
- Updates client, server, WebSocket, and protocol cleanup paths.
- Records the new protected API.
File summaries
| File | Summary |
|---|---|
kotlin-sdk-server/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/server/StreamableHttpServerTransport.kt |
Filters cancellation during stream cleanup. |
kotlin-sdk-core/src/commonTest/kotlin/io/modelcontextprotocol/kotlin/sdk/utils/RunCatchingCancellableTest.kt |
Tests cancellation-aware exception handling. |
kotlin-sdk-core/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/utils/RunCatchingCancellable.kt |
Adds cancellation-preserving result handling. |
kotlin-sdk-core/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/shared/WebSocketMcpTransport.kt |
Treats cancellation as normal closure. |
kotlin-sdk-core/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/shared/Protocol.kt |
Suppresses secondary cancellation failures. |
kotlin-sdk-core/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/shared/AbstractTransport.kt |
Adds safe error-callback invocation. |
kotlin-sdk-core/api/kotlin-sdk-core.api |
Records the protected API addition. |
kotlin-sdk-client/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/client/StdioClientTransport.kt |
Uses cancellation-aware callbacks. |
kotlin-sdk-client/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/client/SseClientTransport.kt |
Preserves cancellation during setup and cleanup. |
kotlin-sdk-client/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/client/Client.kt |
Stops logging initialization cancellation. |
Review details
Suppressed comments (1)
kotlin-sdk-core/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/shared/WebSocketMcpTransport.kt:89
- This changes the WebSocket lifecycle contract, but the added test only exercises
runCatchingCancellable; there is no WebSocket transport test asserting that a cancelled session invokesonClosewithoutonError. Add a regression test for this branch, since it is the behavior changed here and the existing WebSocket coverage only exercises successful connections.
if (it != null && it !is CancellationException) {
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![jetbrains-air[bot]](https://avatars.githubusercontent.com/in/3314082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nice, careful fix — cancellation handling looks right and consistent with the rest of the transports, so this is good to merge.
Produced by Air Automations. Name: Code review / Run: https://air.jetbrains.cloud/org/05cf1a7f-6ab5-713b-abd3-29d0c8a05e2d/automations/409567ec-d3a7-40ac-ba95-8518ec577ac2?run=d4bbb085-17bd-4430-85e3-d5f2767fea4c
| protected fun invokeOnCloseCallback() { | ||
| if (onCloseCalled.compareAndSet(expectedValue = false, newValue = true)) { | ||
| runCatching { _onClose() } | ||
| runCatchingCancellable { _onClose() } |
There was a problem hiding this comment.
Stale statement — kotlin-sdk-core/.../shared/AbstractTransport.kt:66-67, in the KDoc of invokeOnCloseCallback:
Any exceptions thrown during the execution of the
_onClosecallback are caught and suppressed.
What falsified it — this PR swapped runCatching { _onClose() } for runCatchingCancellable { _onClose() } on line 71. runCatchingCancellable re-throws CancellationException instead of capturing it, so invokeOnCloseCallback can now throw, and "any exceptions ... are caught and suppressed" is no longer true. The sibling invokeOnErrorCallback added in the same commit documents exactly this nuance; invokeOnCloseCallback was left with the pre-change wording.
Suggested correction — replace that sentence with wording that matches the new behaviour, e.g.: "Any [Throwable] the _onClose callback raises is caught and suppressed. A [kotlin.coroutines.cancellation.CancellationException] propagates instead."
Produced by Air Automations. Name: Documentation maintenance / Run: https://air.jetbrains.cloud/org/05cf1a7f-6ab5-713b-abd3-29d0c8a05e2d/automations/78771d31-9b0b-450c-810f-118fb9430a87?run=5c807b4e-2447-499a-a7ce-a6c26a496e43
CancellationExceptionno longer reachesonErrorcallbacks or the log.Breaking Changes
None
Types of changes
Checklist