modelcontextprotocol / modelcontextprotocol/kotlin-sdk

onClose callbacks are synchronous, so suspending cleanup has nowhere to run

Open
#919 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

api breaking enhancement P2 ready for work
Dominant language
Kotlin
Stars
1.5k
Forks
248
Avg merge
1d 20h
Merged PRs (30d)
23

Description

Affected version / component: io.modelcontextprotocol:kotlin-sdk 0.14.0 — shared/Transport.kt, shared/AbstractTransport.kt, server/ServerSession.kt, server/Server.kt

Every close callback in the server stack is a plain () -> Unit. Transport.onClose, ServerSession.onClose, and Server.onClose all take a non-suspending block, and Server.close() invokes the registered chain synchronously. That leaves nowhere to run suspending teardown — closing pooled resources, or cancelling and then joining a child CoroutineScope — without wrapping it in runBlocking, which can stall or deadlock the close path.

The contract is synchronous at each layer. AbstractTransport composes the registered blocks as () -> Unit, and invokeOnCloseCallback() runs them inline inside runCatching, swallowing exceptions. ServerSession.onClose chains the same way, and its onClose() hook fires the chain synchronously. Server.onClose does likewise, and Server.close() calls it directly:

public suspend fun close() {
    notificationService.close()
    sessions.forEach { (_, session) -> session.close() }
    _onClose() // synchronous; no suspension point for teardown
}

close() is itself a suspend function, so the synchronous callback is a limitation of the API contract rather than something the implementation forces.

For context, #778 (shipped in 0.13.0) made the stdio transport's own close() suspending, with a graceful drain of in-flight outbound messages, and moved message handlers to Dispatchers.Default. It did not touch the user-facing close callbacks, which are still non-suspending everywhere, and there is no awaitClose() in 0.14.0.

Suggested fix

Offer a suspending close path — for example an onClose(block: suspend () -> Unit) overload, or a suspend close hook that Server.close() and session/transport teardown await. Either lets a server with suspending resources shut down deterministically instead of bridging through runBlocking.

Workaround

Keep the onClose block limited to synchronous work (flipping flags, cancelling a scope without joining it) and move all suspending teardown into a separate suspend function that callers invoke and await after close. Alternatively, treat the callback purely as a signal that resumes a coroutine, and run the suspending cleanup outside it:

suspend fun awaitClose(session: ServerSession) = suspendCoroutine { cont ->
    session.onClose { cont.resume(Unit) }
}
// awaitClose(session); mySuspendingCleanup()

Both work, but they push lifecycle management onto every caller, which is easy to get wrong.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by tracing the synchronous close chains in shared/Transport.kt, shared/AbstractTransport.kt, server/ServerSession.kt, and server/Server.kt, including Server.close(). Define a suspending close-hook contract that preserves teardown ordering and exception handling, then verify that suspending cleanup can be awaited without runBlocking at each transport, session, and server layer.

Written by the indexing model from the issue text.

Assessment

Tech stack
kotlin
Domain
backend-api-design
Issue type
Feature
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.