Skip to content

Conversation

@devcrocod
Copy link
Contributor

Motivation and Context

Sessions were added to the sessions list on connect but never removed when closed, causing a memory leak in long-running server processes. The closed sessions and their closures would accumulate indefinitely, preventing garbage collection.

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

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 adds automatic cleanup of closed sessions from the server's active sessions list. The change ensures that when a session closes, it is automatically removed from the internal sessions list maintained by the Server class.

  • Registers an onClose callback on each session to handle cleanup when the session terminates

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

@devcrocod devcrocod requested a review from Copilot November 3, 2025 15:12
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

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

kotlin-sdk-server/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/server/Server.kt:180

  • The onClose handler is registered before the session is added to the sessions list, creating a race condition. If the session closes during or immediately after session.connect(transport) (line 178), the cleanup handler will attempt to remove a session that hasn't been added yet, and the session will be added to the list (line 180) after it's already closed. This can cause closed sessions to remain in the sessions list indefinitely. Move the session addition (line 180) before registering the onClose handler (lines 173-176) to ensure the session is in the list before the cleanup handler can execute.
        // Register cleanup handler to remove session from list when it closes
        session.onClose {
            logger.debug { "Removing closed session from active sessions list" }
            sessions.update { list -> list - session }
        }
        logger.debug { "Server session connecting to transport" }
        session.connect(transport)
        logger.debug { "Server session successfully connected to transport" }
        sessions.update { it.add(session) }

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

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 merged commit ee341df into main Nov 3, 2025
10 checks passed
@kpavlov kpavlov deleted the devcrocod/cleanup-closedd-sessions branch November 3, 2025 15:36
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.

4 participants