Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Fix logger multi-session cross-talk issue
- 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>
  • Loading branch information
Claude and gkorland committed Feb 23, 2026
commit c20d9dad0ee7a6bae1d560d4e53f13b7dd9f25b9
7 changes: 5 additions & 2 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -162,13 +162,16 @@ async function startHTTPServer(): Promise<void> {
sessionIdGenerator: () => randomUUID(),
});

// Connect a fresh McpServer for this session
const sessionServer = createSessionServer();

transport.onclose = () => {
const sid = transport.sessionId;
if (sid) sessions.delete(sid);
// Remove server from logger to prevent memory leaks and cross-talk
logger.removeMcpServer(sessionServer);
};

// Connect a fresh McpServer for this session
const sessionServer = createSessionServer();
await sessionServer.connect(transport);
await transport.handleRequest(req, res, parsedBody);

Expand Down
55 changes: 55 additions & 0 deletions src/services/logger.service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,61 @@ describe('Logger Service', () => {

expect(() => logger.setMcpServer(mockMcpServer as any)).not.toThrow();
});

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();
});
Comment on lines +121 to +174
Copy link

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
});

describe('Error Resilience', () => {
Expand Down
63 changes: 38 additions & 25 deletions src/services/logger.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,10 @@ type LogLevel = 'DEBUG' | 'INFO' | 'WARN' | 'ERROR';
export class Logger {
private logDir: string;
private logFile: string;
private mcpServer?: McpServer;
private mcpServers: Set<McpServer>;

constructor() {
this.mcpServers = new Set();
// For MCP servers, we primarily use MCP notifications for logging
// File logging is optional and only enabled if environment allows it
this.logDir = userLogDir('falkordb-mcp', 'mulliken-llc', '0.1.0', false, true);
Expand All @@ -42,11 +43,20 @@ export class Logger {
}

/**
* Set the MCP server instance to enable client notifications
* This should be called after the server is created but before starting
* Add an MCP server instance to enable client notifications
* This should be called after each server is created but before starting
* Supports multiple servers for HTTP multi-session deployments
*/
public setMcpServer(server: McpServer): void {
this.mcpServer = server;
this.mcpServers.add(server);
}

/**
* Remove an MCP server instance when a session closes
* This prevents memory leaks and ensures logs aren't sent to closed sessions
*/
public removeMcpServer(server: McpServer): void {
this.mcpServers.delete(server);
}

private formatLog(level: string, message: string, context?: LogContext): string {
Expand Down Expand Up @@ -78,30 +88,33 @@ export class Logger {
}

private async sendMcpLog(level: LogLevel, message: string, context?: LogContext): Promise<void> {
if (!this.mcpServer) {
if (this.mcpServers.size === 0) {
return;
}

try {
// Format log data for MCP client
const logData = context ? `${message} | ${JSON.stringify(context)}` : message;

// Map WARN to warning for MCP spec compliance
const mcpLevel = level === 'WARN' ? 'warning' : level.toLowerCase();

// Send notification to MCP client
await this.mcpServer.server.notification({
method: 'notifications/message',
params: {
level: mcpLevel,
data: logData,
logger: 'falkordb-mcp'
}
});
} catch {
// If MCP notification fails, just continue - file logging is our fallback
// Don't log this error to avoid infinite loops
}
// Format log data once for all servers
const logData = context ? `${message} | ${JSON.stringify(context)}` : message;
const mcpLevel = level === 'WARN' ? 'warning' : level.toLowerCase();

// Send notification to all connected MCP clients
const notifications = Array.from(this.mcpServers).map(async (server) => {
try {
await server.server.notification({
method: 'notifications/message',
params: {
level: mcpLevel,
data: logData,
logger: 'falkordb-mcp'
}
});
} catch {
// If MCP notification fails for one server, continue with others
// Don't log this error to avoid infinite loops
}
});

// Send to all servers in parallel
await Promise.allSettled(notifications);
}

private async log(level: LogLevel, message: string, context?: LogContext): Promise<void> {
Expand Down