Skip to content

Conversation

@devcrocod
Copy link
Contributor

Added awaitCancellation in runSseMcpServerWithPlainConfiguration()

How Has This Been Tested?

Added test for plain configuration server
Checked with inspector

Breaking Changes

None

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

@devcrocod devcrocod requested a review from kpavlov December 8, 2025 16:47
@devcrocod devcrocod marked this pull request as ready for review December 8, 2025 16:51
Copilot AI review requested due to automatic review settings December 8, 2025 16:51
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes the kotlin-mcp-server sample by adding awaitCancellation() to prevent the SSE connection from closing immediately in the plain configuration server. It also adds comprehensive test coverage for both server configuration types.

Key changes:

  • Added awaitCancellation() in the SSE endpoint handler for plain configuration to keep the connection alive
  • Refactored tests to support both Ktor plugin and plain configuration server types through a parameterized approach
  • Changed function signature to return EmbeddedServer<*, *> for consistency between both server implementations

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
samples/kotlin-mcp-server/src/main/kotlin/io/modelcontextprotocol/sample/server/server.kt Added awaitCancellation() to fix SSE connection closing prematurely and changed return type to EmbeddedServer<*, *>
samples/kotlin-mcp-server/src/test/kotlin/McpServerType.kt New enum to define server configuration types with their respective SSE endpoints and factory methods
samples/kotlin-mcp-server/src/test/kotlin/TestEnvironment.kt Refactored from singleton object to parameterized class to support testing multiple server configurations
samples/kotlin-mcp-server/src/test/kotlin/SseServerIntegrationTest.kt Refactored tests into abstract base class with concrete implementations for each server type

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

this.port = port
}
}
val transport = httpClient.mcpSseTransport("http://127.0.0.1:$port/${config.sseEndpoint}")
Copy link

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The URL construction "http://127.0.0.1:$port/${config.sseEndpoint}" creates incorrect URLs:

  • When sseEndpoint = "/sse", it produces "http://127.0.0.1:$port//sse" (double slash)
  • When sseEndpoint = "", it produces "http://127.0.0.1:$port/" (trailing slash)

The sseEndpoint in McpServerType should not include the leading slash. Change it to:

PLAIN_CONFIGURATION(
    sseEndpoint = "sse",
    ...
)

Then this URL construction will work correctly for both cases.

Copilot uses AI. Check for mistakes.
serverFactory = { port -> runSseMcpServerUsingKtorPlugin(port, wait = false) }
),
PLAIN_CONFIGURATION(
sseEndpoint = "/sse",
Copy link

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sseEndpoint should be "sse" instead of "/sse" to avoid creating a double-slash URL (//sse) when concatenated in TestEnvironment.kt line 40. The leading slash is already provided in the URL construction.

Suggested change
sseEndpoint = "/sse",
sseEndpoint = "sse",

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make sense, but can be addressed separately

Copy link
Contributor

@kpavlov kpavlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kpavlov kpavlov force-pushed the devcrocod/fix-sample-plain-server-config branch from 0ed9ccc to 5263bc3 Compare December 8, 2025 20:21
@kpavlov kpavlov merged commit 363eab6 into main Dec 8, 2025
8 checks passed
@kpavlov kpavlov deleted the devcrocod/fix-sample-plain-server-config branch December 8, 2025 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants