Fix logger multi-session cross-talk in HTTP mode#31
Conversation
- Changed Logger to maintain a Set of MCP servers instead of single reference - Added removeMcpServer() method to clean up closed sessions - Updated sendMcpLog() to notify all registered servers in parallel - Updated HTTP session handler to call removeMcpServer() on close - Added tests for multi-session logger behavior - All 110 tests pass successfully Co-authored-by: gkorland <753206+gkorland@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #31 +/- ##
==========================================
+ Coverage 48.32% 49.01% +0.68%
==========================================
Files 11 11
Lines 449 457 +8
Branches 103 103
==========================================
+ Hits 217 224 +7
- Misses 232 233 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical bug where the singleton Logger service was overwriting its internal MCP server reference on each new HTTP session, causing log notifications from all active users to be sent only to the most recently connected user. The solution changes the Logger from tracking a single mcpServer to maintaining a Set<McpServer> to support multiple concurrent HTTP sessions.
Changes:
- Refactored Logger service to use
Set<McpServer>instead of a single server reference, enabling multi-session logging in HTTP mode - Added
removeMcpServer()method and updated session cleanup to prevent memory leaks when sessions close - Added comprehensive test coverage for multi-session scenarios and server removal
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/services/logger.service.ts | Changed from single mcpServer field to Set<McpServer>, updated setMcpServer() to add to Set, added removeMcpServer() for cleanup, and modified sendMcpLog() to broadcast to all registered servers using Promise.allSettled() |
| src/index.ts | Moved sessionServer declaration before transport.onclose callback (to capture in closure) and added logger.removeMcpServer(sessionServer) call in the close handler to clean up terminated sessions |
| src/services/logger.service.test.ts | Added tests for multiple server support and server removal functionality to verify notifications reach all active sessions and removed sessions stop receiving logs |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| it('should support multiple MCP servers for multi-session deployments', async () => { | ||
| const mockServer1 = { | ||
| server: { | ||
| notification: jest.fn().mockResolvedValue(undefined), | ||
| }, | ||
| }; | ||
| const mockServer2 = { | ||
| server: { | ||
| notification: jest.fn().mockResolvedValue(undefined), | ||
| }, | ||
| }; | ||
|
|
||
| // Add both servers | ||
| logger.setMcpServer(mockServer1 as any); | ||
| logger.setMcpServer(mockServer2 as any); | ||
|
|
||
| // Log a message | ||
| await logger.info('Test message for multiple servers'); | ||
|
|
||
| // Both servers should receive the notification | ||
| expect(mockServer1.server.notification).toHaveBeenCalled(); | ||
| expect(mockServer2.server.notification).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('should remove MCP server from notification list', async () => { | ||
| const mockServer1 = { | ||
| server: { | ||
| notification: jest.fn().mockResolvedValue(undefined), | ||
| }, | ||
| }; | ||
| const mockServer2 = { | ||
| server: { | ||
| notification: jest.fn().mockResolvedValue(undefined), | ||
| }, | ||
| }; | ||
|
|
||
| // Add both servers | ||
| logger.setMcpServer(mockServer1 as any); | ||
| logger.setMcpServer(mockServer2 as any); | ||
|
|
||
| // Remove server1 | ||
| logger.removeMcpServer(mockServer1 as any); | ||
|
|
||
| // Clear previous calls | ||
| mockServer1.server.notification.mockClear(); | ||
| mockServer2.server.notification.mockClear(); | ||
|
|
||
| // Log a message | ||
| await logger.info('Test message after removing server'); | ||
|
|
||
| // Only server2 should receive the notification | ||
| expect(mockServer1.server.notification).not.toHaveBeenCalled(); | ||
| expect(mockServer2.server.notification).toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
The tests are adding mock servers to the singleton logger instance without cleaning them up afterward. While this doesn't cause issues with the current test order, it could lead to test pollution if new tests are added later.
Consider adding cleanup in the afterEach hook to reset the logger's server list. This would make tests more isolated and maintainable. For example:
afterEach(() => {
// Clear all registered servers to prevent test pollution
// This assumes we add a method like clearServers() or expose mcpServers for testing
consoleErrorSpy.mockRestore();
});Note: This would require either adding a test-only method to clear servers or making the mcpServers property accessible for testing purposes.
The singleton Logger service was overwriting its internal MCP server reference on each new HTTP session, causing all log notifications to route only to the most recently connected user.
Changes
Logger service: Changed from single
mcpServerreference toSet<McpServer>to track multiple active sessionssetMcpServer()now adds servers to the Set instead of overwritingremoveMcpServer()to clean up closed sessions and prevent memory leakssendMcpLog()broadcasts to all registered servers usingPromise.allSettled()HTTP session handler: Call
removeMcpServer()in thetransport.onclosecallback to remove terminated sessions from the logger's notification listTests: Added multi-session test coverage verifying notifications reach all active sessions and that removed sessions stop receiving logs
Example
Stdio transport mode continues working as before with a single server in the Set.
Original prompt