mirror of
https://github.com/n8n-io/n8n.git
synced 2026-10-17 13:39:44 +02:00
fix(core): Report MCP server connection failures in verify_mcp_server (#40559)
This commit is contained in:
parent
4b5b68ee29
commit
fbbbadc292
|
|
@ -35,6 +35,7 @@ function makeDeps(overrides: Partial<Parameters<typeof buildVerifyMcpServerTool>
|
|||
function makeMcpClient(overrides: Partial<McpClient> = {}): McpClient {
|
||||
return {
|
||||
listTools: vi.fn().mockResolvedValue([]),
|
||||
getConnectionFailures: vi.fn().mockReturnValue([]),
|
||||
close: vi.fn().mockResolvedValue(undefined),
|
||||
...overrides,
|
||||
} as unknown as McpClient;
|
||||
|
|
@ -142,6 +143,59 @@ describe('buildVerifyMcpServerTool', () => {
|
|||
});
|
||||
});
|
||||
|
||||
it('returns { ok: false, error } when the connection fails with no tools', async () => {
|
||||
const mcpClient = makeMcpClient({
|
||||
listTools: vi.fn().mockResolvedValue([]),
|
||||
getConnectionFailures: vi
|
||||
.fn()
|
||||
.mockReturnValue([{ server: 'my-server', error: 'fetch failed' }]),
|
||||
});
|
||||
buildMcpClientForServerMock.mockResolvedValue(mcpClient);
|
||||
|
||||
const tool = buildVerifyMcpServerTool(makeDeps());
|
||||
const result = await tool.handler!(
|
||||
{ name: 'my-server', url: 'https://example.test/mcp' },
|
||||
{} as never,
|
||||
);
|
||||
|
||||
expect(result).toEqual({
|
||||
ok: false,
|
||||
error: 'MCP server "my-server" connection failed: fetch failed',
|
||||
});
|
||||
});
|
||||
|
||||
it('does not apply the credential when the connection fails with no tools', async () => {
|
||||
const applyCredentialToMcpServer = vi.fn();
|
||||
const mcpClient = makeMcpClient({
|
||||
listTools: vi.fn().mockResolvedValue([]),
|
||||
getConnectionFailures: vi
|
||||
.fn()
|
||||
.mockReturnValue([{ server: 'my-server', error: 'fetch failed' }]),
|
||||
});
|
||||
buildMcpClientForServerMock.mockResolvedValue(mcpClient);
|
||||
|
||||
const tool = buildVerifyMcpServerTool(
|
||||
makeDeps({
|
||||
agentId: 'agent-1',
|
||||
applyCredentialToMcpServer,
|
||||
}),
|
||||
);
|
||||
const result = await tool.handler!(
|
||||
{
|
||||
name: 'my-server',
|
||||
url: 'https://example.test/mcp',
|
||||
credential: 'cred-42',
|
||||
},
|
||||
{} as never,
|
||||
);
|
||||
|
||||
expect(result).toEqual({
|
||||
ok: false,
|
||||
error: 'MCP server "my-server" connection failed: fetch failed',
|
||||
});
|
||||
expect(applyCredentialToMcpServer).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('returns { ok: false, error } when listTools throws', async () => {
|
||||
const mcpClient = makeMcpClient({
|
||||
listTools: vi.fn().mockRejectedValue(new Error('connection timeout')),
|
||||
|
|
|
|||
|
|
@ -154,6 +154,17 @@ export function buildVerifyMcpServerTool(deps: VerifyMcpServerDeps): BuiltTool {
|
|||
deps,
|
||||
);
|
||||
const tools = await listToolsWithinDeadline(client, timeoutMs, ctx.abortSignal);
|
||||
const failures = client.getConnectionFailures();
|
||||
if (failures.length > 0) {
|
||||
return {
|
||||
ok: false,
|
||||
error: failures
|
||||
.map(
|
||||
(failure) => `MCP server "${failure.server}" connection failed: ${failure.error}`,
|
||||
)
|
||||
.join('; '),
|
||||
};
|
||||
}
|
||||
const mappedTools = tools.map((t) => ({
|
||||
name: t.mcpToolName ?? t.name,
|
||||
description: t.description ?? '',
|
||||
|
|
|
|||
|
|
@ -2,7 +2,7 @@ import type { CredentialProvider } from '@n8n/agents';
|
|||
import type { AgentJsonMcpServerConfig } from '@n8n/api-types';
|
||||
import type { CustomFetch } from '@n8n/backend-network';
|
||||
import { mock } from 'vitest-mock-extended';
|
||||
import { UserError } from 'n8n-workflow';
|
||||
import { OperationalError, UserError } from 'n8n-workflow';
|
||||
|
||||
import type { OauthService } from '@/oauth/oauth.service';
|
||||
|
||||
|
|
@ -19,10 +19,16 @@ import {
|
|||
const mcpClientCtor = vi.fn();
|
||||
const listToolsMock = vi.fn();
|
||||
const closeMock = vi.fn();
|
||||
const getConnectionFailuresMock = vi.fn();
|
||||
vi.mock('@n8n/agents', () => ({
|
||||
McpClient: vi.fn(function (configs: unknown) {
|
||||
mcpClientCtor(configs);
|
||||
return { configs, close: closeMock, listTools: listToolsMock };
|
||||
return {
|
||||
configs,
|
||||
close: closeMock,
|
||||
listTools: listToolsMock,
|
||||
getConnectionFailures: getConnectionFailuresMock,
|
||||
};
|
||||
}),
|
||||
}));
|
||||
|
||||
|
|
@ -800,6 +806,8 @@ describe('listMcpServerTools', () => {
|
|||
listToolsMock.mockReset();
|
||||
closeMock.mockReset();
|
||||
closeMock.mockResolvedValue(undefined);
|
||||
getConnectionFailuresMock.mockReset();
|
||||
getConnectionFailuresMock.mockReturnValue([]);
|
||||
});
|
||||
|
||||
it('returns name/description pairs (empty description fallback) and closes the client', async () => {
|
||||
|
|
@ -824,6 +832,17 @@ describe('listMcpServerTools', () => {
|
|||
expect(closeMock).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it('rejects a recorded connection failure and still closes the client', async () => {
|
||||
listToolsMock.mockResolvedValue([]);
|
||||
getConnectionFailuresMock.mockReturnValue([{ server: 'srv', error: 'fetch failed' }]);
|
||||
|
||||
const pending = listMcpServerTools(makeServer(), deps());
|
||||
|
||||
await expect(pending).rejects.toBeInstanceOf(OperationalError);
|
||||
await expect(pending).rejects.toThrow('MCP server "srv" connection failed: fetch failed');
|
||||
expect(closeMock).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it('swallows close errors', async () => {
|
||||
listToolsMock.mockResolvedValue([{ name: 'echo', description: 'd' }]);
|
||||
closeMock.mockRejectedValue(new Error('already closed'));
|
||||
|
|
|
|||
|
|
@ -306,6 +306,14 @@ export async function listMcpServerTools(
|
|||
try {
|
||||
client = await buildMcpClientForServer(server, deps);
|
||||
const tools = await client.listTools();
|
||||
const failures = client.getConnectionFailures();
|
||||
if (failures.length > 0) {
|
||||
throw new OperationalError(
|
||||
failures
|
||||
.map((failure) => `MCP server "${failure.server}" connection failed: ${failure.error}`)
|
||||
.join('; '),
|
||||
);
|
||||
}
|
||||
return tools.map((tool) => ({ name: tool.name, description: tool.description ?? '' }));
|
||||
} finally {
|
||||
await client?.close().catch(() => {});
|
||||
|
|
|
|||
|
|
@ -9,6 +9,7 @@ import { Container } from '@n8n/di';
|
|||
import { TELEMETRY_EVENT } from '@n8n/telemetry';
|
||||
import type { Mock } from 'vitest';
|
||||
import { mock } from 'vitest-mock-extended';
|
||||
import { OperationalError } from 'n8n-workflow';
|
||||
|
||||
vi.mock('@/permissions.ee/check-access', () => ({
|
||||
userHasScopes: vi.fn(),
|
||||
|
|
@ -1840,6 +1841,21 @@ describe('McpAgentToolsService', () => {
|
|||
authentication: 'none',
|
||||
};
|
||||
|
||||
it('returns ok: false when the MCP connection fails', async () => {
|
||||
outboundHttp.transport.mockReturnValue({ asCustomFetch: () => vi.fn() } as never);
|
||||
listMcpServerToolsMock.mockRejectedValue(
|
||||
new OperationalError('MCP server "srv" connection failed: fetch failed'),
|
||||
);
|
||||
|
||||
const result = await callTool('verify_agent_mcp_server', input);
|
||||
|
||||
expect(result.structuredContent).toEqual({
|
||||
ok: false,
|
||||
error: 'MCP server "srv" connection failed: fetch failed',
|
||||
});
|
||||
expect(result.isError).toBe(true);
|
||||
});
|
||||
|
||||
it('connects and returns the server tool list', async () => {
|
||||
outboundHttp.transport.mockReturnValue({ asCustomFetch: () => vi.fn() } as never);
|
||||
listMcpServerToolsMock.mockResolvedValue([{ name: 'echo', description: 'Echo' }]);
|
||||
|
|
|
|||
|
|
@ -24,7 +24,7 @@ import type { User } from '@n8n/db';
|
|||
import { Service } from '@n8n/di';
|
||||
import type { Scope } from '@n8n/permissions';
|
||||
import { isRecord } from '@n8n/utils/is-record';
|
||||
import { UserError } from 'n8n-workflow';
|
||||
import { OperationalError, UserError } from 'n8n-workflow';
|
||||
import { z } from 'zod';
|
||||
|
||||
import { CredentialsService } from '@/credentials/credentials.service';
|
||||
|
|
@ -1734,28 +1734,35 @@ export class McpAgentToolsService {
|
|||
await this.requireAccessibleCredential(credentialProvider, input.credential);
|
||||
}
|
||||
|
||||
const tools = await listMcpServerTools(
|
||||
{
|
||||
name: input.name,
|
||||
url: input.url,
|
||||
transport: input.transport,
|
||||
authentication: input.authentication,
|
||||
credential: input.credential,
|
||||
metadata: input.metadata,
|
||||
...(input.connectionTimeoutMs !== undefined
|
||||
? { connectionTimeoutMs: input.connectionTimeoutMs }
|
||||
: {}),
|
||||
},
|
||||
{
|
||||
credentialProvider,
|
||||
oauthService: this.oauthService,
|
||||
projectId: input.projectId,
|
||||
proxyFetch: createAiMcpFetch(this.outboundHttp),
|
||||
resolveRegistryConnection: async (nodeTypeName) =>
|
||||
await this.mcpRegistryService.getConnection(nodeTypeName),
|
||||
},
|
||||
);
|
||||
return { ok: true, tools };
|
||||
try {
|
||||
const tools = await listMcpServerTools(
|
||||
{
|
||||
name: input.name,
|
||||
url: input.url,
|
||||
transport: input.transport,
|
||||
authentication: input.authentication,
|
||||
credential: input.credential,
|
||||
metadata: input.metadata,
|
||||
...(input.connectionTimeoutMs !== undefined
|
||||
? { connectionTimeoutMs: input.connectionTimeoutMs }
|
||||
: {}),
|
||||
},
|
||||
{
|
||||
credentialProvider,
|
||||
oauthService: this.oauthService,
|
||||
projectId: input.projectId,
|
||||
proxyFetch: createAiMcpFetch(this.outboundHttp),
|
||||
resolveRegistryConnection: async (nodeTypeName) =>
|
||||
await this.mcpRegistryService.getConnection(nodeTypeName),
|
||||
},
|
||||
);
|
||||
return { ok: true, tools };
|
||||
} catch (error) {
|
||||
if (error instanceof OperationalError) {
|
||||
return { ok: false, error: error.message };
|
||||
}
|
||||
throw error;
|
||||
}
|
||||
}
|
||||
|
||||
private async updateIntegration(user: User, input: UpdateIntegrationInput) {
|
||||
|
|
|
|||
Loading…
Reference in New Issue
Block a user