[gibkim]

Fixes #1086

Problem

When a message handler threw CancellationException, processorPump rethrew it. The read loop ended, the finally block closed the write channel, and the transport fired onClose without onError. A single handler's cancellation, for example a cancelled request scope leaking out of a handler, closed the whole connection. Other handler exceptions are logged and the loop continues.

Change

• In the handler catch for CancellationException, call currentCoroutineContext().ensureActive() first. If the transport's own coroutine is cancelled (close() or a cancelled external scope), this rethrows and the loop stops as before.
• Otherwise the exception came from the handler. It is logged and passed to onError, like any other handler failure, and the loop continues.
• Follow-up in Protocol.onRequest (core, second commit): in the serial phase before notifications/initialized, a CancellationException leaking from a handler was rethrown. Once the Stdio transport stops closing on it, that request got no reply and the peer waited for its own timeout. It now answers INTERNAL_ERROR, like the concurrent phase, after ensureActive() so a cancelled delivering coroutine still propagates. This touches core and therefore every transport.

Tests

• should continue processing messages after handler throws now also runs with CancellationException("handler cancelled"). On main this case times out because the transport closes after the first message.
• Removed should not invoke onError for CancellationException in handler, which pinned the old behavior.
• New should stop without onError when the scope is cancelled during a handler: an external scope is cancelled while a handler is suspended. The transport must close without reporting an error. Without the ensureActive() call this test fails with the JobCancellationException reported to onError. The test waits for the scope's job after closing the input, because onClose can fire before the processor has handled the cancellation.
• should not hang close when handler suspends indefinitely still passes, so close() during a suspended handler is unaffected.
• :kotlin-sdk-server tests pass on jvmTest, jsNodeTest and macosArm64Test; :integration-test:jvmTest passes. ktlintCheck, detekt and checkKotlinAbi pass. No public API change.
• Core: new handler leaking CancellationException in the serial phase produces INTERNAL_ERROR (fails on the first commit with the rethrown exception) and cancelling the delivering coroutine in the serial phase sends no response. :kotlin-sdk-core, :kotlin-sdk-server and :kotlin-sdk-client jvmTest pass, plus the new tests on jsNodeTest and macosArm64Test; core ktlintCheck, detekt and checkKotlinAbi pass.

🤖 Generated with Claude Code

[Copilot]

Copilot review overview

🟢 Approval recommended

The focused implementation correctly preserves structured cancellation semantics and is covered by targeted regression tests.

Review effort: Balanced
Findings: None

What changed in this PR

Keeps StdioServerTransport active when only a message handler is cancelled, while preserving transport-level cancellation.

Changes:

• Distinguishes handler cancellation from transport cancellation using ensureActive().
• Reports handler cancellations through onError and continues processing.
• Adds regression coverage for both cancellation paths.

File	Description
StdioServerTransport.kt	Handles escaped handler cancellation without stopping the transport.
StdioServerTransportTest.kt	Updates and expands cancellation behavior tests.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[jetbrains-air]

Approving — I've left one non-blocking note inline for you.

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=9ec05c83-ec7c-4a82-afff-a07cf8545630

[jetbrains-air]

Problem: keeping the loop alive is the right call, but it now leaves the request the cancelled handler was serving with no reply at all. In the concurrent dispatch phase Protocol.onRequest already turns a cancellation that escapes a live handler job into an INTERNAL_ERROR response (Protocol.kt:626-632, "so the peer does not hang until its own timeout"). The serial, pre-notifications/initialized phase takes the handlerJob == null branch and rethrows instead — and until this change that rethrow landed here and tore the transport down, so the peer at least observed a closed connection. With the transport surviving, nothing goes out.

Failure scenario: a client sends initialize, then tools/call before the server has processed notifications/initialized (so dispatch is still inline), and the tool handler leaks a cancellation — e.g. an inner withTimeout expiring. I ran this against the branch: the server answers initialize, answers a later unrelated ping, and never answers the tools/call. The client blocks on that call until its own request timeout. On main the same scenario closed the stream, so the client failed fast.

Suggested fix: make the serial branch in Protocol.onRequest answer instead of rethrowing, using the same liveness check as here so a real close() during an inline handler still propagates:

if (handlerJob == null) {
    currentCoroutineContext().ensureActive()
    respondWithError(capturedTransport, request, e)
    return@withContext
}

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=9ec05c83-ec7c-4a82-afff-a07cf8545630

[gibkim]

Fixed in 14c4d86, as suggested: the serial branch in Protocol.onRequest now calls ensureActive() and answers INTERNAL_ERROR instead of rethrowing. Added a core test for the leak (fails on the previous commit) and one that cancels the delivering coroutine and expects no response. This moves part of the fix into core, so I noted that in the PR description.

[Copilot]

🟢 Approval recommended

The cancellation distinction is correctly implemented and covered across transport and protocol layers.

0 open findings

🧠 Review effort: Balanced

---

Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

[jetbrains-air]

Nice work — this reads correct end to end, so happy to approve.

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=5821c7a3-2243-4838-bc0d-67896dadc9d3

[chrikrah]

@gibkim approving at 14c4d86: both production files at the merge base fail one new test each, and on your tree both suites pass.

$ ./gradlew :kotlin-sdk-server:jvmTest --tests "*StdioServerTransportTest" :kotlin-sdk-core:jvmTest --tests "*ProtocolConcurrencyTest*"   # 14c4d86, Temurin 21, Gradle 9.8.0
StdioServerTransportTest[jvm]   tests=24 failures=0
ProtocolConcurrencyTest[jvm]    tests=19 failures=0
$ # same command, Protocol.kt and StdioServerTransport.kt at 32e6483 (merge base), tests kept
StdioServerTransportTest[jvm]   tests=24 failures=1   [3] handler throws CancellationException: Timed out waiting for 5000 ms
ProtocolConcurrencyTest[jvm]    tests=19 failures=1   handler leaking CancellationException in the serial phase produces INTERNAL_ERROR

non-blocking: the body already notes the core change. The title becomes the squash commit, so fix(core,server): ... would carry it into git log as well.

@devcrocod you filed #1086. This is ready for your review, and the merge-base runs above show both new tests failing without the fix.

[gibkim]

Thanks @chrikrah for running the merge-base comparison, that's the check that matters here.

Retitled to fix(core,server): ... so the core change shows up in the squash commit too.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

[jetbrains-air]

Approving — everything checks out here.

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=6583000c-3f12-4881-b698-59324e5d4737

[gibkim]

@devcrocod gentle ping on this one when you have a moment. I've updated the branch with the latest main; CI is green and the earlier approvals still apply. The core change (Protocol.onRequest) is covered by the two new tests.

[jetbrains-air]

Approving — this holds up end to end, nothing blocking from me.

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=220b52a8-6473-44f0-af53-43aaca507413