fix: propagate CancellationException instead of logging as error#706
fix: propagate CancellationException instead of logging as error#706
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #242 by ensuring coroutine cancellations (CancellationException and subclasses) are propagated rather than treated as errors, reducing noisy/incorrect error logging in normal cancellation scenarios.
Changes:
- Rethrow
CancellationExceptionin multiple server/client transports and protocol message handling paths to avoid logging/reporting cancellations as errors. - Make shutdown/cleanup paths non-cancellable in a few transports via
withContext(NonCancellable). - Add protocol tests ensuring cancellation from notification handlers is propagated and does not trigger
onError.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| kotlin-sdk-server/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/server/StreamableHttpServerTransport.kt | Propagates CancellationException in several SSE/HTTP paths; makes close() and SSE stream closing non-cancellable. |
| kotlin-sdk-server/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/server/SSEServerTransport.kt | Propagates CancellationException in request parsing/handling paths. |
| kotlin-sdk-core/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/shared/Protocol.kt | Propagates CancellationException from notification handling and from method-not-found response sending. |
| kotlin-sdk-core/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/shared/WebSocketMcpTransport.kt | Propagates CancellationException from message decoding/dispatch loop. |
| kotlin-sdk-client/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/client/StdioClientTransport.kt | Makes close non-cancellable and propagates CancellationException during message processing. |
| kotlin-sdk-client/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/client/SseClientTransport.kt | Makes close non-cancellable; adjusts shutdown behavior for the collector job. |
| kotlin-sdk-core/src/commonTest/kotlin/io/modelcontextprotocol/kotlin/sdk/shared/ProtocolTest.kt | Adds tests asserting cancellation propagation and correct onError behavior for notifications. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| override suspend fun closeResources() { | ||
| job?.cancelAndJoin() | ||
| try { | ||
| if (::session.isInitialized) session.cancel() | ||
| if (::scope.isInitialized) scope.cancel() | ||
| endpoint.cancel() | ||
| } catch (e: Throwable) { | ||
| _onError(e) | ||
| withContext(NonCancellable) { | ||
| job?.cancel() | ||
| try { | ||
| if (::session.isInitialized) session.cancel() | ||
| if (::scope.isInitialized) scope.cancel() | ||
| endpoint.cancel() | ||
| } catch (e: Throwable) { | ||
| _onError(e) | ||
| } |
There was a problem hiding this comment.
closeResources() now only calls job?.cancel() and does not wait for the collector coroutine to finish. This can make close() return while the transport is still processing events / running cleanup, and can race with session.cancel()/scope.cancel(). Consider cancelling and then join()ing the job when closeResources() is invoked from an external coroutine, while avoiding self-join when called from the collector’s finally block.
e5l
left a comment
e5l
left a comment
There was a problem hiding this comment.
lgtm. should we introduce this inspection as part of the code review skill? it is frequent error
This is already documented in agents.md |
fixes #242
How Has This Been Tested?
new and existing tests pass
Breaking Changes
none
Types of changes
Checklist