From 217e724686031af7a7bacdb17ace093a3475d065 Mon Sep 17 00:00:00 2001 From: Eli Powell Date: Fri, 15 May 2026 18:13:02 -0400 Subject: [PATCH] fix(mcp): preserve not found errors --- .../src/mcp/__tests__/mcp.controller.spec.ts | 61 +++++++++++++++++ apps/api/src/mcp/mcp.controller.ts | 65 ++++++++----------- 2 files changed, 88 insertions(+), 38 deletions(-) create mode 100644 apps/api/src/mcp/__tests__/mcp.controller.spec.ts diff --git a/apps/api/src/mcp/__tests__/mcp.controller.spec.ts b/apps/api/src/mcp/__tests__/mcp.controller.spec.ts new file mode 100644 index 00000000..f9212a2a --- /dev/null +++ b/apps/api/src/mcp/__tests__/mcp.controller.spec.ts @@ -0,0 +1,61 @@ +/* eslint-disable @typescript-eslint/no-explicit-any */ +import { HttpException, HttpStatus, NotFoundException } from '@nestjs/common'; +import { McpController } from '../mcp.controller'; + +describe('McpController', () => { + let registry: { get: jest.Mock }; + let metricsService: { getHealthSummary: jest.Mock }; + let controller: McpController; + + beforeEach(() => { + registry = { + get: jest.fn().mockReturnValue({ + getInfoParsed: jest.fn().mockResolvedValue({ server: {} }), + }), + }; + metricsService = { + getHealthSummary: jest.fn().mockResolvedValue({ status: 'ok' }), + }; + + controller = new McpController( + registry as any, + metricsService as any, + {} as any, + {} as any, + {} as any, + {} as any, + {} as any, + ); + }); + + it('preserves not found errors from connection lookup', async () => { + registry.get.mockImplementationOnce(() => { + throw new NotFoundException("Connection 'missing' not found."); + }); + + await expect(controller.getInfo('missing')).rejects.toBeInstanceOf(NotFoundException); + }); + + it('preserves not found errors from services that resolve an instance id', async () => { + metricsService.getHealthSummary.mockRejectedValueOnce( + new NotFoundException("Connection 'missing' not found."), + ); + + await expect(controller.getHealth('missing')).rejects.toBeInstanceOf(NotFoundException); + }); + + it('still wraps non-http failures in a generic 500', async () => { + registry.get.mockImplementationOnce(() => { + throw new Error('socket closed'); + }); + + try { + await controller.getInfo('conn-1'); + throw new Error('Expected getInfo to fail'); + } catch (error) { + expect(error).toBeInstanceOf(HttpException); + expect((error as HttpException).message).toBe('Failed to get info'); + expect((error as HttpException).getStatus()).toBe(HttpStatus.INTERNAL_SERVER_ERROR); + } + }); +}); diff --git a/apps/api/src/mcp/mcp.controller.ts b/apps/api/src/mcp/mcp.controller.ts index c7e4a8f5..830700d4 100644 --- a/apps/api/src/mcp/mcp.controller.ts +++ b/apps/api/src/mcp/mcp.controller.ts @@ -39,6 +39,14 @@ export class McpController { this.telemetryService = telemetryService ?? null; } + private throwMcpError(error: unknown, logMessage: string, fallbackMessage: string): never { + if (error instanceof HttpException) { + throw error; + } + this.logger.error(logMessage, error instanceof Error ? error.stack : error); + throw new HttpException(fallbackMessage, HttpStatus.INTERNAL_SERVER_ERROR); + } + @Get('instances') async listInstances() { const list = this.registry.list(); @@ -62,8 +70,7 @@ export class McpController { const info = await client.getInfoParsed(); return info; } catch (error) { - this.logger.error(`Failed to get info for ${id}`, error instanceof Error ? error.stack : error); - throw new HttpException('Failed to get info', HttpStatus.INTERNAL_SERVER_ERROR); + this.throwMcpError(error, `Failed to get info for ${id}`, 'Failed to get info'); } } @@ -74,8 +81,7 @@ export class McpController { const parsedCount = safeLimit(count, 25); return await client.getSlowLog(parsedCount); } catch (error) { - this.logger.error(`Failed to get slowlog for ${id}`, error instanceof Error ? error.stack : error); - throw new HttpException('Failed to get slowlog', HttpStatus.INTERNAL_SERVER_ERROR); + this.throwMcpError(error, `Failed to get slowlog for ${id}`, 'Failed to get slowlog'); } } @@ -85,8 +91,7 @@ export class McpController { const client = this.registry.get(id); return await client.getLatestLatencyEvents(); } catch (error) { - this.logger.error(`Failed to get latency for ${id}`, error instanceof Error ? error.stack : error); - throw new HttpException('Failed to get latency', HttpStatus.INTERNAL_SERVER_ERROR); + this.throwMcpError(error, `Failed to get latency for ${id}`, 'Failed to get latency'); } } @@ -100,8 +105,7 @@ export class McpController { ]); return { doctor, stats }; } catch (error) { - this.logger.error(`Failed to get memory for ${id}`, error instanceof Error ? error.stack : error); - throw new HttpException('Failed to get memory diagnostics', HttpStatus.INTERNAL_SERVER_ERROR); + this.throwMcpError(error, `Failed to get memory for ${id}`, 'Failed to get memory diagnostics'); } } @@ -120,8 +124,7 @@ export class McpController { if (msg.includes('unknown command') || msg.includes('COMMANDLOG')) { return { entries: [], note: 'COMMANDLOG not available on this instance' }; } - this.logger.error(`Failed to get commandlog for ${id}`, error instanceof Error ? error.stack : error); - throw new HttpException('Failed to get commandlog', HttpStatus.INTERNAL_SERVER_ERROR); + this.throwMcpError(error, `Failed to get commandlog for ${id}`, 'Failed to get commandlog'); } } @@ -131,8 +134,7 @@ export class McpController { const client = this.registry.get(id); return await client.getClients(); } catch (error) { - this.logger.error(`Failed to get clients for ${id}`, error instanceof Error ? error.stack : error); - throw new HttpException('Failed to get clients', HttpStatus.INTERNAL_SERVER_ERROR); + this.throwMcpError(error, `Failed to get clients for ${id}`, 'Failed to get clients'); } } @@ -145,8 +147,7 @@ export class McpController { const parsedLimit = limit !== undefined ? safeLimit(limit, MAX_LIMIT) : undefined; return await this.metricsService.getSlowLogPatternAnalysis(parsedLimit, id); } catch (error) { - this.logger.error(`Failed to get slowlog patterns for ${id}`, error instanceof Error ? error.stack : error); - throw new HttpException('Failed to get slowlog patterns', HttpStatus.INTERNAL_SERVER_ERROR); + this.throwMcpError(error, `Failed to get slowlog patterns for ${id}`, 'Failed to get slowlog patterns'); } } @@ -174,8 +175,7 @@ export class McpController { if (msg.includes('unknown command') || msg.includes('COMMANDLOG')) { return { entries: [], note: 'COMMANDLOG not available on this instance' }; } - this.logger.error(`Failed to get commandlog history for ${id}`, error instanceof Error ? error.stack : error); - throw new HttpException('Failed to get commandlog history', HttpStatus.INTERNAL_SERVER_ERROR); + this.throwMcpError(error, `Failed to get commandlog history for ${id}`, 'Failed to get commandlog history'); } } @@ -198,8 +198,7 @@ export class McpController { if (msg.includes('unknown command') || msg.includes('COMMANDLOG')) { return { entries: [], note: 'COMMANDLOG not available on this instance' }; } - this.logger.error(`Failed to get commandlog patterns for ${id}`, error instanceof Error ? error.stack : error); - throw new HttpException('Failed to get commandlog patterns', HttpStatus.INTERNAL_SERVER_ERROR); + this.throwMcpError(error, `Failed to get commandlog patterns for ${id}`, 'Failed to get commandlog patterns'); } } @@ -228,8 +227,7 @@ export class McpController { id, ); } catch (error) { - this.logger.error(`Failed to get anomalies for ${id}`, error instanceof Error ? error.stack : error); - throw new HttpException('Failed to get anomalies', HttpStatus.INTERNAL_SERVER_ERROR); + this.throwMcpError(error, `Failed to get anomalies for ${id}`, 'Failed to get anomalies'); } } @@ -250,8 +248,7 @@ export class McpController { id, ); } catch (error) { - this.logger.error(`Failed to get client activity for ${id}`, error instanceof Error ? error.stack : error); - throw new HttpException('Failed to get client activity', HttpStatus.INTERNAL_SERVER_ERROR); + this.throwMcpError(error, `Failed to get client activity for ${id}`, 'Failed to get client activity'); } } @@ -264,8 +261,7 @@ export class McpController { if (msg.includes('CLUSTERDOWN') || msg.includes('cluster mode')) { return { error: 'not_cluster', message: 'This instance is not running in cluster mode.' }; } - this.logger.error(`Failed to get cluster nodes for ${id}`, error instanceof Error ? error.stack : error); - throw new HttpException('Failed to get cluster nodes', HttpStatus.INTERNAL_SERVER_ERROR); + this.throwMcpError(error, `Failed to get cluster nodes for ${id}`, 'Failed to get cluster nodes'); } } @@ -278,8 +274,7 @@ export class McpController { if (msg.includes('CLUSTERDOWN') || msg.includes('cluster mode')) { return { error: 'not_cluster', message: 'This instance is not running in cluster mode.' }; } - this.logger.error(`Failed to get cluster node stats for ${id}`, error instanceof Error ? error.stack : error); - throw new HttpException('Failed to get cluster node stats', HttpStatus.INTERNAL_SERVER_ERROR); + this.throwMcpError(error, `Failed to get cluster node stats for ${id}`, 'Failed to get cluster node stats'); } } @@ -296,8 +291,7 @@ export class McpController { if (msg.includes('CLUSTERDOWN') || msg.includes('cluster mode')) { return { error: 'not_cluster', message: 'This instance is not running in cluster mode.' }; } - this.logger.error(`Failed to get cluster slowlog for ${id}`, error instanceof Error ? error.stack : error); - throw new HttpException('Failed to get cluster slowlog', HttpStatus.INTERNAL_SERVER_ERROR); + this.throwMcpError(error, `Failed to get cluster slowlog for ${id}`, 'Failed to get cluster slowlog'); } } @@ -321,8 +315,7 @@ export class McpController { if (msg.includes('CLUSTERDOWN') || msg.includes('cluster mode')) { return { error: 'not_cluster', message: 'This instance is not running in cluster mode.' }; } - this.logger.error(`Failed to get cluster slot stats for ${id}`, error instanceof Error ? error.stack : error); - throw new HttpException('Failed to get cluster slot stats', HttpStatus.INTERNAL_SERVER_ERROR); + this.throwMcpError(error, `Failed to get cluster slot stats for ${id}`, 'Failed to get cluster slot stats'); } } @@ -337,8 +330,7 @@ export class McpController { try { return await this.metricsService.getLatencyHistory(eventName, id); } catch (error) { - this.logger.error(`Failed to get latency history for ${id}/${eventName}`, error instanceof Error ? error.stack : error); - throw new HttpException('Failed to get latency history', HttpStatus.INTERNAL_SERVER_ERROR); + this.throwMcpError(error, `Failed to get latency history for ${id}/${eventName}`, 'Failed to get latency history'); } } @@ -361,8 +353,7 @@ export class McpController { connectionId: id, }); } catch (error) { - this.logger.error(`Failed to get audit entries for ${id}`, error instanceof Error ? error.stack : error); - throw new HttpException('Failed to get audit entries', HttpStatus.INTERNAL_SERVER_ERROR); + this.throwMcpError(error, `Failed to get audit entries for ${id}`, 'Failed to get audit entries'); } } @@ -383,8 +374,7 @@ export class McpController { latest: true, }); } catch (error) { - this.logger.error(`Failed to get hot keys for ${id}`, error instanceof Error ? error.stack : error); - throw new HttpException('Failed to get hot keys', HttpStatus.INTERNAL_SERVER_ERROR); + this.throwMcpError(error, `Failed to get hot keys for ${id}`, 'Failed to get hot keys'); } } @@ -393,8 +383,7 @@ export class McpController { try { return await this.metricsService.getHealthSummary(id); } catch (error) { - this.logger.error(`Failed to get health for ${id}`, error instanceof Error ? error.stack : error); - throw new HttpException('Failed to get health', HttpStatus.INTERNAL_SERVER_ERROR); + this.throwMcpError(error, `Failed to get health for ${id}`, 'Failed to get health'); } }