Skip to content

fix(mcp): preserve not found errors #204

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
epowell40 wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix(mcp): preserve not found errors #204

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
61 changes: 61 additions & 0 deletions apps/api/src/mcp/__tests__/mcp.controller.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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);
}
});
});
65 changes: 27 additions & 38 deletions apps/api/src/mcp/mcp.controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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');
}
}

Expand All @@ -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');
}
}

Expand All @@ -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');
}
}

Expand All @@ -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');
}
}

Expand All @@ -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');
}
}

Expand All @@ -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');
}
}

Expand All @@ -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');
}
}

Expand Down Expand Up @@ -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');
}
}

Expand All @@ -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');
}
}

Expand Down Expand Up @@ -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');
}
}

Expand All @@ -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');
}
}

Expand All @@ -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');
}
}

Expand All @@ -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');
}
}

Expand All @@ -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');
}
}

Expand All @@ -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');
}
}

Expand All @@ -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');
}
}

Expand All @@ -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');
}
}

Expand All @@ -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');
}
}

Expand All @@ -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');
}
}

Expand Down
Loading