From e72a777d25b0abfb8722c4810d70e05c8204ccff Mon Sep 17 00:00:00 2001 From: Justin Blumencranz <96924014+j15z@users.noreply.github.com> Date: Sat, 11 Jul 2026 10:50:57 -0700 Subject: [PATCH 1/2] fix(mcp): improve OAuth failure recovery --- .../settings/components/mcp/mcp.tsx | 53 +++---- .../mcp/refresh-action-state.test.ts | 47 ++++++ .../components/mcp/refresh-action-state.ts | 37 +++++ .../components/mcp/server-tools-label.test.ts | 18 +++ .../components/mcp/server-tools-label.ts | 20 +++ .../settings-header/settings-header.tsx | 8 +- apps/sim/hooks/queries/mcp.test.tsx | 143 +++++++++++++++++ apps/sim/hooks/queries/mcp.ts | 12 +- apps/sim/lib/mcp/client.test.ts | 82 +++++++++- apps/sim/lib/mcp/client.ts | 69 ++++++++- apps/sim/lib/mcp/service.test.ts | 62 +++++++- apps/sim/lib/mcp/service.ts | 145 ++++++++++-------- 12 files changed, 587 insertions(+), 109 deletions(-) create mode 100644 apps/sim/app/workspace/[workspaceId]/settings/components/mcp/refresh-action-state.test.ts create mode 100644 apps/sim/app/workspace/[workspaceId]/settings/components/mcp/refresh-action-state.ts create mode 100644 apps/sim/app/workspace/[workspaceId]/settings/components/mcp/server-tools-label.test.ts create mode 100644 apps/sim/app/workspace/[workspaceId]/settings/components/mcp/server-tools-label.ts create mode 100644 apps/sim/hooks/queries/mcp.test.tsx diff --git a/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/mcp.tsx b/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/mcp.tsx index 9bcdb853fad..171df562caf 100644 --- a/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/mcp.tsx +++ b/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/mcp.tsx @@ -44,6 +44,8 @@ import { useWorkflowRegistry } from '@/stores/workflows/registry/store' import { useSubBlockStore } from '@/stores/workflows/subblock/store' import type { BlockState } from '@/stores/workflows/workflow/types' import { McpServerFormModal } from './components' +import { getRefreshActionState } from './refresh-action-state' +import { getServerToolsLabel } from './server-tools-label' const logger = createLogger('McpSettings') @@ -58,16 +60,6 @@ function formatTransportLabel(transport: string): string { .join('-') } -function formatToolsLabel(tools: McpTool[], connectionStatus?: string): string { - if (connectionStatus === 'error') { - return 'Unable to connect' - } - const count = tools.length - const plural = count !== 1 ? 's' : '' - const names = count > 0 ? `: ${tools.map((t) => t.name).join(', ')}` : '' - return `${count} tool${plural}${names}` -} - interface ServerListItemProps { server: McpServer tools: McpTool[] @@ -88,7 +80,7 @@ function ServerListItem({ onViewDetails, }: ServerListItemProps) { const transportLabel = formatTransportLabel(server.transport || 'http') - const toolsLabel = formatToolsLabel(tools, server.connectionStatus) + const toolsLabel = getServerToolsLabel(tools, server.connectionStatus, server.lastError) const isError = server.connectionStatus === 'error' return ( @@ -295,19 +287,11 @@ export function MCP() { } useEffect(() => { - if (!refreshServerMutation.isSuccess) return + if (!refreshServerMutation.isSuccess && !refreshServerMutation.isError) return const timeout = window.setTimeout(() => refreshServerMutation.reset(), 3000) return () => window.clearTimeout(timeout) - // eslint-disable-next-line react-hooks/exhaustive-deps -- mutation object is unstable; isSuccess flag is the trigger - }, [refreshServerMutation.isSuccess]) - - const refreshingServerId = refreshServerMutation.isPending - ? refreshServerMutation.variables?.serverId - : null - const refreshedServerId = refreshServerMutation.isSuccess - ? refreshServerMutation.variables?.serverId - : null - const refreshedWorkflowsUpdated = refreshServerMutation.data?.workflowsUpdated + // eslint-disable-next-line react-hooks/exhaustive-deps -- mutation object is unstable; status flags are the triggers + }, [refreshServerMutation.isSuccess, refreshServerMutation.isError]) const editingServer = editingServerId ? (servers.find((s) => s.id === editingServerId) as McpServer | undefined) @@ -372,15 +356,12 @@ export function MCP() { if (selectedServer) { const { server, tools } = selectedServer const transportLabel = formatTransportLabel(server.transport || 'http') - - const refreshLabel = - refreshingServerId === server.id - ? 'Refreshing...' - : refreshedServerId === server.id - ? refreshedWorkflowsUpdated - ? `Synced (${refreshedWorkflowsUpdated} workflow${refreshedWorkflowsUpdated === 1 ? '' : 's'})` - : 'Refreshed' - : 'Refresh tools' + const isCurrentRefresh = refreshServerMutation.variables?.serverId === server.id + const refreshAction = getRefreshActionState({ + mutationStatus: isCurrentRefresh ? refreshServerMutation.status : 'idle', + connectionStatus: isCurrentRefresh ? refreshServerMutation.data?.status : undefined, + workflowsUpdated: isCurrentRefresh ? refreshServerMutation.data?.workflowsUpdated : undefined, + }) return ( handleRefreshServer(server.id), - disabled: refreshingServerId === server.id || refreshedServerId === server.id, + disabled: refreshAction.disabled, }, { text: 'Edit', @@ -634,7 +616,10 @@ export function MCP() { tools={tools} isDeleting={deletingServers.has(server.id)} isLoadingTools={isLoadingTools} - isRefreshing={refreshingServerId === server.id} + isRefreshing={ + refreshServerMutation.isPending && + refreshServerMutation.variables?.serverId === server.id + } onRemove={() => handleRemoveServer(server.id)} onViewDetails={() => handleViewDetails(server.id)} /> diff --git a/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/refresh-action-state.test.ts b/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/refresh-action-state.test.ts new file mode 100644 index 00000000000..de27fb0cdc6 --- /dev/null +++ b/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/refresh-action-state.test.ts @@ -0,0 +1,47 @@ +import { describe, expect, it } from 'vitest' +import { getRefreshActionState } from './refresh-action-state' + +describe('getRefreshActionState', () => { + it.each(['error', 'disconnected'] as const)( + 'shows a retryable red-text Failed state when refresh returns %s', + (status) => { + expect( + getRefreshActionState({ + mutationStatus: 'success', + connectionStatus: status, + workflowsUpdated: 0, + }) + ).toEqual({ + text: 'Failed', + textTone: 'error', + disabled: false, + }) + } + ) + + it('shows Failed when the refresh request itself rejects', () => { + expect( + getRefreshActionState({ + mutationStatus: 'error', + }) + ).toEqual({ + text: 'Failed', + textTone: 'error', + disabled: false, + }) + }) + + it('preserves successful refresh feedback', () => { + expect( + getRefreshActionState({ + mutationStatus: 'success', + connectionStatus: 'connected', + workflowsUpdated: 2, + }) + ).toEqual({ + text: 'Synced (2 workflows)', + textTone: undefined, + disabled: true, + }) + }) +}) diff --git a/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/refresh-action-state.ts b/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/refresh-action-state.ts new file mode 100644 index 00000000000..28a6869921e --- /dev/null +++ b/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/refresh-action-state.ts @@ -0,0 +1,37 @@ +import type { MutationStatus } from '@tanstack/react-query' +import type { RefreshMcpServerResult } from '@/lib/api/contracts/mcp' +import type { SettingsAction } from '@/app/workspace/[workspaceId]/settings/components/settings-header/settings-header' + +interface RefreshActionStateInput { + mutationStatus: MutationStatus + connectionStatus?: RefreshMcpServerResult['status'] + workflowsUpdated?: number +} + +type RefreshActionState = Pick + +export function getRefreshActionState({ + mutationStatus, + connectionStatus, + workflowsUpdated, +}: RefreshActionStateInput): RefreshActionState { + if (mutationStatus === 'pending') { + return { text: 'Refreshing...', textTone: undefined, disabled: true } + } + + if ( + mutationStatus === 'error' || + (mutationStatus === 'success' && connectionStatus !== 'connected') + ) { + return { text: 'Failed', textTone: 'error', disabled: false } + } + + if (mutationStatus === 'success') { + const text = workflowsUpdated + ? `Synced (${workflowsUpdated} workflow${workflowsUpdated === 1 ? '' : 's'})` + : 'Refreshed' + return { text, textTone: undefined, disabled: true } + } + + return { text: 'Refresh tools', textTone: undefined, disabled: false } +} diff --git a/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/server-tools-label.test.ts b/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/server-tools-label.test.ts new file mode 100644 index 00000000000..3143a30fe8a --- /dev/null +++ b/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/server-tools-label.test.ts @@ -0,0 +1,18 @@ +import { describe, expect, it } from 'vitest' +import { getServerToolsLabel } from './server-tools-label' + +describe('getServerToolsLabel', () => { + it('shows the persisted server error for errored connections', () => { + expect(getServerToolsLabel([], 'error', 'MCP error -32001: Request timed out')).toBe( + 'MCP error -32001: Request timed out' + ) + }) + + it('falls back when an errored connection has no persisted error', () => { + expect(getServerToolsLabel([], 'error', null)).toBe('Unable to connect') + }) + + it('continues showing discovered tools for healthy connections', () => { + expect(getServerToolsLabel([{ name: 'search' }], 'connected', null)).toBe('1 tool: search') + }) +}) diff --git a/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/server-tools-label.ts b/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/server-tools-label.ts new file mode 100644 index 00000000000..0f74eaa6256 --- /dev/null +++ b/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/server-tools-label.ts @@ -0,0 +1,20 @@ +import type { McpServer } from '@/lib/api/contracts/mcp' + +interface NamedTool { + name: string +} + +export function getServerToolsLabel( + tools: NamedTool[], + connectionStatus?: McpServer['connectionStatus'], + lastError?: McpServer['lastError'] +): string { + if (connectionStatus === 'error') { + return lastError?.trim() || 'Unable to connect' + } + + const count = tools.length + const plural = count !== 1 ? 's' : '' + const names = count > 0 ? `: ${tools.map((tool) => tool.name).join(', ')}` : '' + return `${count} tool${plural}${names}` +} diff --git a/apps/sim/app/workspace/[workspaceId]/settings/components/settings-header/settings-header.tsx b/apps/sim/app/workspace/[workspaceId]/settings/components/settings-header/settings-header.tsx index 52d3a19cbd7..5f555eb9ff2 100644 --- a/apps/sim/app/workspace/[workspaceId]/settings/components/settings-header/settings-header.tsx +++ b/apps/sim/app/workspace/[workspaceId]/settings/components/settings-header/settings-header.tsx @@ -21,6 +21,7 @@ const useIsomorphicLayoutEffect = typeof window === 'undefined' ? useEffect : us /** The strict contract for a settings header action — rendered as a {@link Chip}, data only. */ export interface SettingsAction { text: string + textTone?: 'error' icon?: ComponentType<{ className?: string }> variant?: 'primary' | 'destructive' active?: boolean @@ -83,6 +84,7 @@ function computeSignature(c: SettingsHeaderConfig): string { b: c.back ? [c.back.text, c.back.icon ? 1 : 0] : null, a: c.actions?.map((x) => [ x.text, + x.textTone ?? '', x.variant ?? '', x.active ?? false, x.disabled ?? false, @@ -173,7 +175,11 @@ export function SettingsHeaderShell({ children }: { children: ReactNode }) { } disabled={action.disabled} > - {action.text} + {action.textTone === 'error' ? ( + {action.text} + ) : ( + action.text + )} ) return action.tooltip ? ( diff --git a/apps/sim/hooks/queries/mcp.test.tsx b/apps/sim/hooks/queries/mcp.test.tsx new file mode 100644 index 00000000000..0175f269315 --- /dev/null +++ b/apps/sim/hooks/queries/mcp.test.tsx @@ -0,0 +1,143 @@ +/** + * @vitest-environment jsdom + */ +import { act, type ReactNode } from 'react' +import { sleep } from '@sim/utils/helpers' +import { QueryClient, QueryClientProvider } from '@tanstack/react-query' +import { createRoot, type Root } from 'react-dom/client' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +const { mockRequestJson } = vi.hoisted(() => ({ + mockRequestJson: vi.fn(), +})) + +vi.mock('@/lib/api/client/request', () => ({ + requestJson: mockRequestJson, +})) + +import { + discoverMcpToolsContract, + listMcpServersContract, + type McpServer, +} from '@/lib/api/contracts/mcp' +import { useMcpToolsQuery } from '@/hooks/queries/mcp' + +const WORKSPACE_ID = 'workspace-1' + +function server(id: string, overrides: Partial = {}): McpServer { + return { + id, + workspaceId: WORKSPACE_ID, + name: id, + transport: 'streamable-http', + url: `https://${id}.example.com/mcp`, + enabled: true, + connectionStatus: 'connected', + createdAt: '2026-01-01T00:00:00.000Z', + updatedAt: '2026-01-01T00:00:00.000Z', + ...overrides, + } +} + +function renderHookWithClient(useHook: () => T): { unmount: () => void } { + ;(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true + const queryClient = new QueryClient({ + defaultOptions: { queries: { retry: false } }, + }) + const container = document.createElement('div') + const root: Root = createRoot(container) + + function Probe() { + useHook() + return null + } + + function Wrapper({ children }: { children: ReactNode }) { + return {children} + } + + act(() => { + root.render( + + + + ) + }) + + return { unmount: () => act(() => root.unmount()) } +} + +async function flush() { + await act(async () => { + for (let i = 0; i < 5; i++) { + await Promise.resolve() + await sleep(0) + } + }) +} + +function mockServers(servers: McpServer[]) { + mockRequestJson.mockImplementation(async (contract) => { + if (contract === listMcpServersContract) { + return { success: true, data: { servers } } + } + if (contract === discoverMcpToolsContract) { + return { success: true, data: { tools: [], totalCount: 0, byServer: {} } } + } + throw new Error('Unexpected MCP request') + }) +} + +describe('useMcpToolsQuery', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + afterEach(() => { + vi.restoreAllMocks() + }) + + it('does not auto-discover disconnected or errored OAuth servers', async () => { + mockServers([ + server('oauth-disconnected', { authType: 'oauth', connectionStatus: 'disconnected' }), + server('oauth-error', { authType: 'oauth', connectionStatus: 'error' }), + ]) + + const { unmount } = renderHookWithClient(() => useMcpToolsQuery(WORKSPACE_ID)) + await flush() + + expect(mockRequestJson).toHaveBeenCalledTimes(1) + expect(mockRequestJson).toHaveBeenCalledWith( + listMcpServersContract, + expect.objectContaining({ query: { workspaceId: WORKSPACE_ID } }) + ) + + unmount() + }) + + it('continues discovering connected OAuth and disconnected non-OAuth servers', async () => { + mockServers([ + server('oauth-connected', { authType: 'oauth', connectionStatus: 'connected' }), + server('headers-disconnected', { authType: 'headers', connectionStatus: 'disconnected' }), + ]) + + const { unmount } = renderHookWithClient(() => useMcpToolsQuery(WORKSPACE_ID)) + await flush() + + expect(mockRequestJson).toHaveBeenCalledTimes(3) + expect(mockRequestJson).toHaveBeenCalledWith( + discoverMcpToolsContract, + expect.objectContaining({ + query: { workspaceId: WORKSPACE_ID, serverId: 'oauth-connected' }, + }) + ) + expect(mockRequestJson).toHaveBeenCalledWith( + discoverMcpToolsContract, + expect.objectContaining({ + query: { workspaceId: WORKSPACE_ID, serverId: 'headers-disconnected' }, + }) + ) + + unmount() + }) +}) diff --git a/apps/sim/hooks/queries/mcp.ts b/apps/sim/hooks/queries/mcp.ts index 42bc6e04b33..8bbbedff3f8 100644 --- a/apps/sim/hooks/queries/mcp.ts +++ b/apps/sim/hooks/queries/mcp.ts @@ -132,13 +132,19 @@ async function fetchMcpTools( export function useMcpToolsQuery(workspaceId: string) { const { data: servers, isLoading: serversLoading } = useMcpServers(workspaceId) - // Skip disabled rows (would 404 → negative-cache) and rows from a previous - // workspace (keepPreviousData on useMcpServers). + // Skip disabled rows (would 404 → negative-cache), rows from a previous + // workspace (keepPreviousData on useMcpServers), and OAuth rows that need an + // explicit authorization/reconnect before discovery can succeed. const serverIds = useMemo( () => servers ? servers - .filter((s) => s.enabled && s.workspaceId === workspaceId) + .filter( + (s) => + s.enabled && + s.workspaceId === workspaceId && + (s.authType !== 'oauth' || s.connectionStatus === 'connected') + ) .map((s) => s.id) .sort() : [], diff --git a/apps/sim/lib/mcp/client.test.ts b/apps/sim/lib/mcp/client.test.ts index 31a5f038a2f..f2077ece1f2 100644 --- a/apps/sim/lib/mcp/client.test.ts +++ b/apps/sim/lib/mcp/client.test.ts @@ -3,6 +3,20 @@ */ import { beforeEach, describe, expect, it, vi } from 'vitest' +const { mockLogger, mockSdkConnect } = vi.hoisted(() => ({ + mockLogger: { + debug: vi.fn(), + error: vi.fn(), + info: vi.fn(), + warn: vi.fn(), + }, + mockSdkConnect: vi.fn().mockResolvedValue(undefined), +})) + +vi.mock('@sim/logger', () => ({ + createLogger: () => mockLogger, +})) + /** * Capture the notification handler registered via `client.setNotificationHandler()`. * This lets us simulate the MCP SDK delivering a `tools/list_changed` notification. @@ -14,7 +28,7 @@ vi.mock('@modelcontextprotocol/sdk/client/index.js', () => ({ class { constructor() { Object.assign(this, { - connect: vi.fn().mockResolvedValue(undefined), + connect: mockSdkConnect, close: vi.fn().mockResolvedValue(undefined), getServerVersion: vi.fn().mockReturnValue('2025-06-18'), getServerCapabilities: vi.fn().mockReturnValue({ tools: { listChanged: true } }), @@ -65,6 +79,7 @@ describe('McpClient notification handler', () => { beforeEach(() => { capturedNotificationHandler = null vi.clearAllMocks() + mockSdkConnect.mockResolvedValue(undefined) }) it('fires onToolsChanged when a notification arrives while connected', async () => { @@ -115,6 +130,71 @@ describe('McpClient notification handler', () => { expect(capturedNotificationHandler).toBeNull() }) + it('uses the server connection timeout for the initialize request', async () => { + const client = new McpClient({ + config: { ...createConfig(), timeout: 12_345 }, + securityPolicy: { requireConsent: false, auditLevel: 'basic' }, + }) + + await client.connect() + + expect(mockSdkConnect).toHaveBeenCalledWith(expect.anything(), { timeout: 12_345 }) + }) + + it('logs connection diagnostics without header values', async () => { + const client = new McpClient({ + config: { + ...createConfig(), + authType: 'headers', + headers: { Authorization: 'Bearer do-not-log', 'X-API-Key': 'also-secret' }, + timeout: 12_345, + }, + securityPolicy: { requireConsent: false, auditLevel: 'basic' }, + }) + + await client.connect() + + expect(mockLogger.info).toHaveBeenCalledWith( + expect.stringContaining('Successfully connected'), + expect.objectContaining({ + authType: 'headers', + headerNames: ['Authorization', 'X-API-Key'], + hasUnresolvedEnvRefs: false, + phase: 'initialize', + outcome: 'connected', + timeoutMs: 12_345, + }) + ) + expect(JSON.stringify(mockLogger.info.mock.calls)).not.toContain('do-not-log') + expect(JSON.stringify(mockLogger.info.mock.calls)).not.toContain('also-secret') + }) + + it('classifies initialize timeouts in connection diagnostics', async () => { + mockSdkConnect.mockRejectedValueOnce(new Error('MCP error -32001: Request timed out')) + const client = new McpClient({ + config: { + ...createConfig(), + headers: { Authorization: 'Bearer do-not-log' }, + }, + securityPolicy: { requireConsent: false, auditLevel: 'basic' }, + }) + + await expect(client.connect()).rejects.toThrow('Request timed out') + + expect(mockLogger.error).toHaveBeenCalledWith( + expect.stringContaining('Failed to connect'), + expect.objectContaining({ + phase: 'initialize', + outcome: 'timeout', + timeoutMs: 30_000, + error: expect.objectContaining({ + message: 'MCP error -32001: Request timed out', + }), + }) + ) + expect(JSON.stringify(mockLogger.error.mock.calls)).not.toContain('do-not-log') + }) + it('passes configured headers for OAuth transports as well as header auth transports', () => { const authProvider = {} as unknown as NonNullable new McpClient({ diff --git a/apps/sim/lib/mcp/client.ts b/apps/sim/lib/mcp/client.ts index 569320ef882..e3221093c67 100644 --- a/apps/sim/lib/mcp/client.ts +++ b/apps/sim/lib/mcp/client.ts @@ -9,9 +9,10 @@ import { ToolListChangedNotificationSchema, } from '@modelcontextprotocol/sdk/types.js' import { createLogger } from '@sim/logger' -import { getErrorMessage } from '@sim/utils/errors' +import { describeError, getErrorMessage } from '@sim/utils/errors' import { getMaxExecutionTimeout } from '@/lib/core/execution-limits' import { createPinnedFetch } from '@/lib/core/security/input-validation.server' +import { sanitizeForLogging } from '@/lib/core/security/redaction' import { McpOauthRedirectRequired } from '@/lib/mcp/oauth' import { type McpClientOptions, @@ -29,9 +30,30 @@ import { type McpVersionInfo, } from '@/lib/mcp/types' import { MCP_CLIENT_CONSTANTS } from '@/lib/mcp/utils' +import { createEnvVarPattern } from '@/executor/utils/reference-validation' const logger = createLogger('McpClient') +type ConnectionOutcome = + | 'started' + | 'connected' + | 'authorization_required' + | 'timeout' + | 'unauthorized' + | 'cancelled' + | 'error' + +function classifyConnectionOutcome(error: unknown): ConnectionOutcome { + if (error instanceof McpOauthRedirectRequired || error instanceof UnauthorizedError) { + return 'authorization_required' + } + const message = getErrorMessage(error, '').toLowerCase() + if (message.includes('connection attempt cancelled')) return 'cancelled' + if (message.includes('timeout') || message.includes('timed out')) return 'timeout' + if (message.includes('401') || message.includes('unauthorized')) return 'unauthorized' + return 'error' +} + interface McpClientConnectOptions { isCancelled?: () => boolean } @@ -90,10 +112,30 @@ export class McpClient { * for `notifications/tools/list_changed` after connecting. */ async connect(options: McpClientConnectOptions = {}): Promise { - logger.info(`Connecting to MCP server: ${this.config.name} (${this.config.transport})`) + const startedAt = Date.now() + const timeoutMs = this.config.timeout ?? MCP_CLIENT_CONSTANTS.CLIENT_TIMEOUT + const headerNames = Object.keys(this.config.headers ?? {}).sort() + const hasUnresolvedEnvRefs = [ + this.config.url ?? '', + ...Object.values(this.config.headers ?? {}), + ].some((value) => createEnvVarPattern().test(value)) + const diagnostics = { + serverId: this.config.id, + authType: this.config.authType ?? (headerNames.length > 0 ? 'headers' : 'none'), + headerNames, + hasUnresolvedEnvRefs, + phase: 'initialize', + timeoutMs, + } + logger.info(`Connecting to MCP server: ${this.config.name} (${this.config.transport})`, { + ...diagnostics, + outcome: 'started' satisfies ConnectionOutcome, + }) try { - await this.client.connect(this.transport) + await this.client.connect(this.transport, { + timeout: timeoutMs, + }) if (options.isCancelled?.()) { await this.client.close().catch((error) => { logger.warn(`Error closing cancelled connection to ${this.config.name}:`, error) @@ -116,17 +158,34 @@ export class McpClient { const serverVersion = this.client.getServerVersion() logger.info(`Successfully connected to MCP server: ${this.config.name}`, { + ...diagnostics, + durationMs: Date.now() - startedAt, + outcome: 'connected' satisfies ConnectionOutcome, protocolVersion: serverVersion, }) } catch (error) { this.isConnected = false + const errorMessage = getErrorMessage(error, 'Unknown error') + const describedError = describeError(error) + logger.error(`Failed to connect to MCP server ${this.config.name}`, { + ...diagnostics, + durationMs: Date.now() - startedAt, + error: { + ...describedError, + message: sanitizeForLogging(describedError.message, 500), + causeChain: describedError.causeChain?.map((cause) => sanitizeForLogging(cause, 500)), + stack: + error instanceof Error && error.stack + ? sanitizeForLogging(error.stack.split('\n').slice(1).join('\n'), 2000) + : undefined, + }, + outcome: classifyConnectionOutcome(error), + }) if (error instanceof McpOauthRedirectRequired || error instanceof UnauthorizedError) { this.connectionStatus.lastError = undefined throw error } - const errorMessage = getErrorMessage(error, 'Unknown error') this.connectionStatus.lastError = errorMessage - logger.error(`Failed to connect to MCP server ${this.config.name}:`, error) throw new McpConnectionError(errorMessage, this.config.name) } } diff --git a/apps/sim/lib/mcp/service.test.ts b/apps/sim/lib/mcp/service.test.ts index f4e5817f69e..3d84da348f7 100644 --- a/apps/sim/lib/mcp/service.test.ts +++ b/apps/sim/lib/mcp/service.test.ts @@ -14,6 +14,7 @@ const { mockValidateSsrf, mockIsDomainAllowed, mockCacheAdapter, + mockUpdateSet, } = vi.hoisted(() => { const mockListTools = vi.fn() const mockConnect = vi.fn() @@ -67,6 +68,7 @@ const { mockValidateDomain: vi.fn(), mockValidateSsrf: vi.fn(), mockIsDomainAllowed: vi.fn(() => true), + mockUpdateSet: vi.fn().mockReturnValue({ where: vi.fn().mockResolvedValue(undefined) }), } }) @@ -80,13 +82,12 @@ vi.mock('@sim/db', () => { }) return thenable } - const setter = vi.fn().mockReturnValue({ where: vi.fn().mockResolvedValue(undefined) }) return { db: { select: vi.fn().mockReturnValue({ from: vi.fn().mockReturnValue({ where }), }), - update: vi.fn().mockReturnValue({ set: setter }), + update: vi.fn().mockReturnValue({ set: mockUpdateSet }), insert: vi.fn(), delete: vi.fn(), }, @@ -366,4 +367,61 @@ describe('McpService.discoverTools per-server caching', () => { expect(after.map((t) => t.name)).toEqual(['a1']) expect(mockListTools).toHaveBeenCalledTimes(1) }) + + it('persists a per-server discovery failure before rethrowing it', async () => { + mockGetWorkspaceServersRows.mockResolvedValue([ + dbRow('mcp-a', 'A', { + statusConfig: { consecutiveFailures: 0, lastSuccessfulDiscovery: null }, + }), + ]) + mockListTools.mockRejectedValueOnce(new Error('Request timed out')) + + await expect(mcpService.discoverServerTools(USER_ID, 'mcp-a', WORKSPACE_ID)).rejects.toThrow( + 'Request timed out' + ) + + expect(mockUpdateSet).toHaveBeenCalledWith( + expect.objectContaining({ + connectionStatus: 'disconnected', + lastError: 'Request timed out', + statusConfig: { consecutiveFailures: 1, lastSuccessfulDiscovery: null }, + }) + ) + }) + + it('promotes the persisted server status to error on the third consecutive failure', async () => { + mockGetWorkspaceServersRows.mockResolvedValue([ + dbRow('mcp-a', 'A', { + statusConfig: { consecutiveFailures: 2, lastSuccessfulDiscovery: null }, + }), + ]) + mockListTools.mockRejectedValueOnce(new Error('Connection refused')) + + await expect(mcpService.discoverServerTools(USER_ID, 'mcp-a', WORKSPACE_ID)).rejects.toThrow( + 'Connection refused' + ) + + expect(mockUpdateSet).toHaveBeenCalledWith( + expect.objectContaining({ + connectionStatus: 'error', + statusConfig: { consecutiveFailures: 3, lastSuccessfulDiscovery: null }, + }) + ) + }) + + it('persists OAuth-required discovery as disconnected without a failure error', async () => { + mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A')]) + mockListTools.mockRejectedValueOnce(new McpOauthAuthorizationRequiredError('mcp-a', 'A')) + + await expect(mcpService.discoverServerTools(USER_ID, 'mcp-a', WORKSPACE_ID)).rejects.toThrow( + 'OAuth authorization required' + ) + + expect(mockUpdateSet).toHaveBeenCalledWith( + expect.objectContaining({ + connectionStatus: 'disconnected', + lastError: null, + }) + ) + }) }) diff --git a/apps/sim/lib/mcp/service.ts b/apps/sim/lib/mcp/service.ts index 7e97d3fc2de..b93d4c8a344 100644 --- a/apps/sim/lib/mcp/service.ts +++ b/apps/sim/lib/mcp/service.ts @@ -5,7 +5,7 @@ import { mcpServers } from '@sim/db/schema' import { createLogger } from '@sim/logger' import { getErrorMessage } from '@sim/utils/errors' import { sleep } from '@sim/utils/helpers' -import { and, eq, isNull } from 'drizzle-orm' +import { and, eq, isNotNull, isNull, ne, or } from 'drizzle-orm' import { isTest } from '@/lib/core/config/env-flags' import { generateRequestId } from '@/lib/core/utils/request' import { McpClient } from '@/lib/mcp/client' @@ -66,6 +66,10 @@ type DiscoveryOutcome = // exemption survives the getErrorMessage call. | { kind: 'error'; message: string; originalError: unknown } +function isOauthAuthorizationError(error: unknown): boolean { + return error instanceof McpOauthAuthorizationRequiredError || error instanceof UnauthorizedError +} + class McpService { private cacheAdapter: McpCacheStorageAdapter private readonly cacheTimeout = MCP_CONSTANTS.CACHE_TIMEOUT @@ -316,6 +320,27 @@ class McpService { toolCount?: number ): Promise { try { + const now = new Date() + + if (success) { + await db + .update(mcpServers) + .set({ + connectionStatus: 'connected', + lastConnected: now, + lastError: null, + toolCount: toolCount ?? 0, + lastToolsRefresh: now, + statusConfig: { + consecutiveFailures: 0, + lastSuccessfulDiscovery: now.toISOString(), + }, + updatedAt: now, + }) + .where(eq(mcpServers.id, serverId)) + return + } + const [currentServer] = await db .select({ statusConfig: mcpServers.statusConfig }) .from(mcpServers) @@ -337,46 +362,24 @@ class McpService { lastSuccessfulDiscovery: storedConfig?.lastSuccessfulDiscovery ?? null, } - const now = new Date() - - if (success) { - await db - .update(mcpServers) - .set({ - connectionStatus: 'connected', - lastConnected: now, - lastError: null, - toolCount: toolCount ?? 0, - lastToolsRefresh: now, - statusConfig: { - consecutiveFailures: 0, - lastSuccessfulDiscovery: now.toISOString(), - }, - updatedAt: now, - }) - .where(eq(mcpServers.id, serverId)) - } else { - const newFailures = currentConfig.consecutiveFailures + 1 - const isErrorState = newFailures >= MCP_CONSTANTS.MAX_CONSECUTIVE_FAILURES - - await db - .update(mcpServers) - .set({ - connectionStatus: isErrorState ? 'error' : 'disconnected', - lastError: error || 'Unknown error', - statusConfig: { - consecutiveFailures: newFailures, - lastSuccessfulDiscovery: currentConfig.lastSuccessfulDiscovery, - }, - updatedAt: now, - }) - .where(eq(mcpServers.id, serverId)) + const newFailures = currentConfig.consecutiveFailures + 1 + const isErrorState = newFailures >= MCP_CONSTANTS.MAX_CONSECUTIVE_FAILURES + + await db + .update(mcpServers) + .set({ + connectionStatus: isErrorState ? 'error' : 'disconnected', + lastError: error || 'Unknown error', + statusConfig: { + consecutiveFailures: newFailures, + lastSuccessfulDiscovery: currentConfig.lastSuccessfulDiscovery, + }, + updatedAt: now, + }) + .where(eq(mcpServers.id, serverId)) - if (isErrorState) { - logger.warn( - `Server ${serverId} marked as error after ${newFailures} consecutive failures` - ) - } + if (isErrorState) { + logger.warn(`Server ${serverId} marked as error after ${newFailures} consecutive failures`) } } catch (err) { logger.error(`Failed to update server status for ${serverId}:`, err) @@ -392,7 +395,7 @@ class McpService { serverId: string, error: unknown ): Promise { - if (error instanceof McpOauthAuthorizationRequiredError || error instanceof UnauthorizedError) { + if (isOauthAuthorizationError(error)) { return } try { @@ -406,6 +409,32 @@ class McpService { } } + private async markServerOauthPending(serverId: string, workspaceId: string): Promise { + try { + await db + .update(mcpServers) + .set({ + connectionStatus: 'disconnected', + lastError: null, + updatedAt: new Date(), + }) + .where( + and( + eq(mcpServers.id, serverId), + eq(mcpServers.workspaceId, workspaceId), + isNull(mcpServers.deletedAt), + or( + isNull(mcpServers.connectionStatus), + ne(mcpServers.connectionStatus, 'disconnected'), + isNotNull(mcpServers.lastError) + ) + ) + ) + } catch (error) { + logger.warn(`Failed to mark OAuth server ${serverId} disconnected:`, error) + } + } + private async isServerUnhealthy(workspaceId: string, serverId: string): Promise { try { const entry = await this.cacheAdapter.get(failureCacheKey(workspaceId, serverId)) @@ -479,10 +508,7 @@ class McpService { await client.disconnect() } } catch (error) { - if ( - error instanceof McpOauthAuthorizationRequiredError || - error instanceof UnauthorizedError - ) { + if (isOauthAuthorizationError(error)) { return { kind: 'oauth-pending' } } return { @@ -535,20 +561,7 @@ class McpService { if (outcome.kind === 'oauth-pending') { // Mark disconnected so the UI surfaces the re-auth button. logger.info(`[${requestId}] Skipping server ${server.name}: OAuth authorization pending`) - deferredSideEffects.push( - db - .update(mcpServers) - .set({ - connectionStatus: 'disconnected', - lastError: null, - updatedAt: new Date(), - }) - .where(eq(mcpServers.id, server.id)) - .then(() => undefined) - .catch((err) => { - logger.warn(`[${requestId}] Failed to mark server ${server.id} disconnected:`, err) - }) - ) + deferredSideEffects.push(this.markServerOauthPending(server.id, workspaceId)) return } if (outcome.kind === 'unhealthy') { @@ -701,6 +714,14 @@ class McpService { continue } // Drop positive cache so a follow-up doesn't return stale tools. + const statusUpdate = isOauthAuthorizationError(error) + ? this.markServerOauthPending(serverId, workspaceId) + : this.updateServerStatus( + serverId, + workspaceId, + false, + getErrorMessage(error, 'Connection failed') + ) await Promise.allSettled([ this.cacheAdapter .delete(serverCacheKey(workspaceId, serverId)) @@ -708,6 +729,7 @@ class McpService { logger.warn(`[${requestId}] Cache delete failed for ${serverId}:`, err) ), this.markServerUnhealthy(workspaceId, serverId, error), + statusUpdate, ]) throw error } @@ -747,10 +769,7 @@ class McpService { error: undefined, }) } catch (error) { - if ( - error instanceof McpOauthAuthorizationRequiredError || - error instanceof UnauthorizedError - ) { + if (isOauthAuthorizationError(error)) { summaries.push({ id: config.id, name: config.name, From 97b3e73ac30af0be09b55f827af6d4a209479246 Mon Sep 17 00:00:00 2001 From: Justin Blumencranz <96924014+j15z@users.noreply.github.com> Date: Sat, 11 Jul 2026 16:22:28 -0700 Subject: [PATCH 2/2] fix(mcp): address OAuth recovery review findings --- .../mcp/servers/[id]/refresh/route.test.ts | 108 ++++++++++++++ .../app/api/mcp/servers/[id]/refresh/route.ts | 68 +++++---- .../settings/components/mcp/mcp.tsx | 13 +- .../mcp/refresh-action-state.test.ts | 2 +- .../components/mcp/server-tools-label.test.ts | 10 +- .../components/mcp/server-tools-label.ts | 4 + apps/sim/hooks/queries/mcp.test.tsx | 79 +++++++++- apps/sim/hooks/queries/mcp.ts | 40 +++-- apps/sim/lib/mcp/client.test.ts | 30 +++- apps/sim/lib/mcp/client.ts | 24 +-- apps/sim/lib/mcp/service.test.ts | 25 +++- apps/sim/lib/mcp/service.ts | 137 ++++++++++++------ 12 files changed, 422 insertions(+), 118 deletions(-) create mode 100644 apps/sim/app/api/mcp/servers/[id]/refresh/route.test.ts diff --git a/apps/sim/app/api/mcp/servers/[id]/refresh/route.test.ts b/apps/sim/app/api/mcp/servers/[id]/refresh/route.test.ts new file mode 100644 index 00000000000..7daed4706c9 --- /dev/null +++ b/apps/sim/app/api/mcp/servers/[id]/refresh/route.test.ts @@ -0,0 +1,108 @@ +/** + * @vitest-environment node + */ +import type { NextRequest } from 'next/server' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { mockDiscoverServerTools, mockSelect, mockUpdateSet } = vi.hoisted(() => ({ + mockDiscoverServerTools: vi.fn(), + mockSelect: vi.fn(), + mockUpdateSet: vi.fn(), +})) + +vi.mock('@sim/db', () => ({ + db: { + select: mockSelect, + update: vi.fn().mockReturnValue({ set: mockUpdateSet }), + }, +})) + +vi.mock('@/lib/core/utils/with-route-handler', () => ({ + withRouteHandler: (handler: unknown) => handler, +})) + +vi.mock('@/lib/mcp/middleware', () => ({ + withMcpAuth: + () => + ( + handler: ( + request: NextRequest, + context: { userId: string; workspaceId: string; requestId: string }, + routeContext: { params: Promise<{ id: string }> } + ) => Promise + ) => + (request: NextRequest, routeContext: { params: Promise<{ id: string }> }) => + handler( + request, + { userId: 'user-1', workspaceId: 'workspace-1', requestId: 'request-1' }, + routeContext + ), +})) + +vi.mock('@/lib/mcp/service', () => ({ + mcpService: { + clearCache: vi.fn(), + discoverServerTools: mockDiscoverServerTools, + }, +})) + +import { POST } from '@/app/api/mcp/servers/[id]/refresh/route' + +const initialServer = { + id: 'server-1', + workspaceId: 'workspace-1', + name: 'OAuth Server', + url: 'https://example.com/mcp', + connectionStatus: 'connected', + lastError: null, + lastConnected: new Date('2026-01-01T00:00:00.000Z'), + toolCount: 4, + statusConfig: { consecutiveFailures: 0, lastSuccessfulDiscovery: null }, +} + +const persistedServer = { + ...initialServer, + connectionStatus: 'disconnected', + lastError: null, + toolCount: 0, +} + +function selectRows(rows: unknown[]) { + return { + from: vi.fn().mockReturnValue({ + where: vi.fn().mockReturnValue({ + limit: vi.fn().mockResolvedValue(rows), + }), + }), + } +} + +describe('MCP server refresh route', () => { + beforeEach(() => { + vi.clearAllMocks() + mockSelect.mockReturnValueOnce(selectRows([initialServer])) + mockUpdateSet.mockReturnValue({ + where: vi.fn().mockReturnValue({ returning: vi.fn().mockResolvedValue([persistedServer]) }), + }) + }) + + it('preserves the service-persisted OAuth pending status', async () => { + mockDiscoverServerTools.mockRejectedValueOnce(new Error('OAuth authorization required')) + + const request = new Request('http://localhost/api/mcp/servers/server-1/refresh', { + method: 'POST', + }) as NextRequest + const response = await POST(request, { params: Promise.resolve({ id: 'server-1' }) }) + const body = await response.json() + + expect(body.data).toEqual( + expect.objectContaining({ + status: 'disconnected', + error: null, + }) + ) + expect(mockUpdateSet).not.toHaveBeenCalledWith( + expect.objectContaining({ connectionStatus: expect.anything() }) + ) + }) +}) diff --git a/apps/sim/app/api/mcp/servers/[id]/refresh/route.ts b/apps/sim/app/api/mcp/servers/[id]/refresh/route.ts index 9f216ebf959..5af4c31ed77 100644 --- a/apps/sim/app/api/mcp/servers/[id]/refresh/route.ts +++ b/apps/sim/app/api/mcp/servers/[id]/refresh/route.ts @@ -1,7 +1,8 @@ import { db } from '@sim/db' import { mcpServers, workflow, workflowBlocks } from '@sim/db/schema' import { createLogger } from '@sim/logger' -import { toError } from '@sim/utils/errors' +import { getErrorMessage, toError } from '@sim/utils/errors' +import { truncate } from '@sim/utils/string' import { and, eq, inArray, isNull } from 'drizzle-orm' import type { NextRequest } from 'next/server' import { mcpServerIdParamsSchema } from '@/lib/api/contracts/mcp' @@ -9,7 +10,7 @@ import { validationErrorResponse } from '@/lib/api/server' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' import { withMcpAuth } from '@/lib/mcp/middleware' import { mcpService } from '@/lib/mcp/service' -import type { McpServerStatusConfig, McpTool, McpToolSchema } from '@/lib/mcp/types' +import type { McpTool, McpToolSchema } from '@/lib/mcp/types' import { createMcpErrorResponse, createMcpSuccessResponse, @@ -184,17 +185,9 @@ export const POST = withRouteHandler( ) } - let connectionStatus: 'connected' | 'disconnected' | 'error' = 'error' - let toolCount = 0 - let lastError: string | null = null let syncResult: SyncResult = { updatedCount: 0, updatedWorkflowIds: [] } let discoveredTools: McpTool[] = [] - - const currentStatusConfig: McpServerStatusConfig = - (server.statusConfig as McpServerStatusConfig | null) ?? { - consecutiveFailures: 0, - lastSuccessfulDiscovery: null, - } + let discoveryError: string | null = null try { discoveredTools = await mcpService.discoverServerTools( @@ -203,10 +196,19 @@ export const POST = withRouteHandler( workspaceId, true ) - connectionStatus = 'connected' - toolCount = discoveredTools.length - logger.info(`[${requestId}] Discovered ${toolCount} tools from server ${serverId}`) + logger.info( + `[${requestId}] Discovered ${discoveredTools.length} tools from server ${serverId}` + ) + } catch (error) { + discoveryError = truncate( + getErrorMessage(error, 'Connection failed').split('\n')[0], + 200, + '' + ) + logger.warn(`[${requestId}] Failed to connect to server ${serverId}:`, error) + } + if (discoveryError === null) { syncResult = await syncToolSchemasToWorkflows( workspaceId, serverId, @@ -214,37 +216,33 @@ export const POST = withRouteHandler( requestId, { url: server.url ?? undefined, name: server.name ?? undefined } ) - } catch (error) { - connectionStatus = 'error' - lastError = - error instanceof Error - ? error.message.split('\n')[0].slice(0, 200) - : 'Connection failed' - logger.warn(`[${requestId}] Failed to connect to server ${serverId}:`, error) } const now = new Date() - const newStatusConfig = - connectionStatus === 'connected' - ? { consecutiveFailures: 0, lastSuccessfulDiscovery: now.toISOString() } - : { - consecutiveFailures: currentStatusConfig.consecutiveFailures + 1, - lastSuccessfulDiscovery: currentStatusConfig.lastSuccessfulDiscovery, - } const [refreshedServer] = await db .update(mcpServers) .set({ lastToolsRefresh: now, - connectionStatus, - lastError, - lastConnected: connectionStatus === 'connected' ? now : server.lastConnected, - toolCount, - statusConfig: newStatusConfig, updatedAt: now, }) - .where(eq(mcpServers.id, serverId)) - .returning() + .where( + and( + eq(mcpServers.id, serverId), + eq(mcpServers.workspaceId, workspaceId), + isNull(mcpServers.deletedAt) + ) + ) + .returning({ + connectionStatus: mcpServers.connectionStatus, + lastConnected: mcpServers.lastConnected, + lastError: mcpServers.lastError, + toolCount: mcpServers.toolCount, + }) + + const connectionStatus = refreshedServer?.connectionStatus ?? 'error' + const lastError = refreshedServer ? refreshedServer.lastError : discoveryError + const toolCount = refreshedServer?.toolCount ?? discoveredTools.length if (connectionStatus === 'connected') { await mcpService.clearCache(workspaceId) diff --git a/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/mcp.tsx b/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/mcp.tsx index 171df562caf..49063bf7536 100644 --- a/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/mcp.tsx +++ b/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/mcp.tsx @@ -21,6 +21,8 @@ import { mcpServerIdParam, mcpServerIdUrlKeys, } from '@/app/workspace/[workspaceId]/settings/[section]/search-params' +import { getRefreshActionState } from '@/app/workspace/[workspaceId]/settings/components/mcp/refresh-action-state' +import { getServerToolsLabel } from '@/app/workspace/[workspaceId]/settings/components/mcp/server-tools-label' import { RowActionsMenu } from '@/app/workspace/[workspaceId]/settings/components/row-actions-menu' import { SettingsEmptyState } from '@/app/workspace/[workspaceId]/settings/components/settings-empty-state' import { SettingsPanel } from '@/app/workspace/[workspaceId]/settings/components/settings-panel' @@ -44,8 +46,6 @@ import { useWorkflowRegistry } from '@/stores/workflows/registry/store' import { useSubBlockStore } from '@/stores/workflows/subblock/store' import type { BlockState } from '@/stores/workflows/workflow/types' import { McpServerFormModal } from './components' -import { getRefreshActionState } from './refresh-action-state' -import { getServerToolsLabel } from './server-tools-label' const logger = createLogger('McpSettings') @@ -81,7 +81,8 @@ function ServerListItem({ }: ServerListItemProps) { const transportLabel = formatTransportLabel(server.transport || 'http') const toolsLabel = getServerToolsLabel(tools, server.connectionStatus, server.lastError) - const isError = server.connectionStatus === 'error' + const hasConnectionIssue = + server.connectionStatus === 'error' || server.connectionStatus === 'disconnected' return (
@@ -95,7 +96,7 @@ function ServerListItem({

{isRefreshing @@ -399,11 +400,11 @@ export function MCP() {

)} - {server.connectionStatus === 'error' && ( + {server.connectionStatus !== 'connected' && (
Status

- {server.lastError || 'Unable to connect'} + {getServerToolsLabel([], server.connectionStatus, server.lastError)}

)} diff --git a/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/refresh-action-state.test.ts b/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/refresh-action-state.test.ts index de27fb0cdc6..a9eeb89513e 100644 --- a/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/refresh-action-state.test.ts +++ b/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/refresh-action-state.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from 'vitest' -import { getRefreshActionState } from './refresh-action-state' +import { getRefreshActionState } from '@/app/workspace/[workspaceId]/settings/components/mcp/refresh-action-state' describe('getRefreshActionState', () => { it.each(['error', 'disconnected'] as const)( diff --git a/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/server-tools-label.test.ts b/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/server-tools-label.test.ts index 3143a30fe8a..e65dadf68e4 100644 --- a/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/server-tools-label.test.ts +++ b/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/server-tools-label.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from 'vitest' -import { getServerToolsLabel } from './server-tools-label' +import { getServerToolsLabel } from '@/app/workspace/[workspaceId]/settings/components/mcp/server-tools-label' describe('getServerToolsLabel', () => { it('shows the persisted server error for errored connections', () => { @@ -12,6 +12,14 @@ describe('getServerToolsLabel', () => { expect(getServerToolsLabel([], 'error', null)).toBe('Unable to connect') }) + it('shows a disconnected state when OAuth was not completed', () => { + expect(getServerToolsLabel([], 'disconnected', null)).toBe('Not Connected') + }) + + it('shows the persisted error for disconnected connections', () => { + expect(getServerToolsLabel([], 'disconnected', 'Request timed out')).toBe('Request timed out') + }) + it('continues showing discovered tools for healthy connections', () => { expect(getServerToolsLabel([{ name: 'search' }], 'connected', null)).toBe('1 tool: search') }) diff --git a/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/server-tools-label.ts b/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/server-tools-label.ts index 0f74eaa6256..f9d8d4896af 100644 --- a/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/server-tools-label.ts +++ b/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/server-tools-label.ts @@ -13,6 +13,10 @@ export function getServerToolsLabel( return lastError?.trim() || 'Unable to connect' } + if (connectionStatus === 'disconnected') { + return lastError?.trim() || 'Not Connected' + } + const count = tools.length const plural = count !== 1 ? 's' : '' const names = count > 0 ? `: ${tools.map((tool) => tool.name).join(', ')}` : '' diff --git a/apps/sim/hooks/queries/mcp.test.tsx b/apps/sim/hooks/queries/mcp.test.tsx index 0175f269315..18617f45b67 100644 --- a/apps/sim/hooks/queries/mcp.test.tsx +++ b/apps/sim/hooks/queries/mcp.test.tsx @@ -20,7 +20,7 @@ import { listMcpServersContract, type McpServer, } from '@/lib/api/contracts/mcp' -import { useMcpToolsQuery } from '@/hooks/queries/mcp' +import { useForceRefreshMcpTools, useMcpServers, useMcpToolsQuery } from '@/hooks/queries/mcp' const WORKSPACE_ID = 'workspace-1' @@ -39,16 +39,20 @@ function server(id: string, overrides: Partial = {}): McpServer { } } -function renderHookWithClient(useHook: () => T): { unmount: () => void } { +function renderHookWithClient(useHook: () => T): { + getResult: () => T + unmount: () => void +} { ;(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } }, }) const container = document.createElement('div') const root: Root = createRoot(container) + let result: T | undefined function Probe() { - useHook() + result = useHook() return null } @@ -64,14 +68,20 @@ function renderHookWithClient(useHook: () => T): { unmount: () => void } { ) }) - return { unmount: () => act(() => root.unmount()) } + return { + getResult: () => { + if (result === undefined) throw new Error('Hook result is not ready') + return result + }, + unmount: () => act(() => root.unmount()), + } } async function flush() { await act(async () => { for (let i = 0; i < 5; i++) { await Promise.resolve() - await sleep(0) + await sleep(1) } }) } @@ -140,4 +150,63 @@ describe('useMcpToolsQuery', () => { unmount() }) + + it('refreshes the server list after a connected OAuth discovery fails', async () => { + let serverListRequests = 0 + mockRequestJson.mockImplementation(async (contract) => { + if (contract === listMcpServersContract) { + serverListRequests++ + const connectionStatus = serverListRequests === 1 ? 'connected' : 'disconnected' + return { + success: true, + data: { + servers: [server('oauth-server', { authType: 'oauth', connectionStatus })], + }, + } + } + if (contract === discoverMcpToolsContract) { + throw new Error('OAuth authorization required') + } + throw new Error('Unexpected MCP request') + }) + + const { unmount } = renderHookWithClient(() => useMcpToolsQuery(WORKSPACE_ID)) + await flush() + + expect(serverListRequests).toBe(2) + expect( + mockRequestJson.mock.calls.filter(([contract]) => contract === discoverMcpToolsContract) + ).toHaveLength(1) + + unmount() + }) + + it('does not force-refresh disconnected OAuth servers', async () => { + mockServers([ + server('oauth-disconnected', { authType: 'oauth', connectionStatus: 'disconnected' }), + server('headers-connected', { authType: 'headers', connectionStatus: 'connected' }), + ]) + + const { getResult, unmount } = renderHookWithClient(() => ({ + servers: useMcpServers(WORKSPACE_ID), + refresh: useForceRefreshMcpTools(), + })) + await flush() + + await act(async () => { + await getResult().refresh.mutateAsync(WORKSPACE_ID) + }) + + const discoveryCalls = mockRequestJson.mock.calls.filter( + ([contract]) => contract === discoverMcpToolsContract + ) + expect(discoveryCalls).toHaveLength(1) + expect(discoveryCalls[0]?.[1]).toEqual( + expect.objectContaining({ + query: { workspaceId: WORKSPACE_ID, refresh: true, serverId: 'headers-connected' }, + }) + ) + + unmount() + }) }) diff --git a/apps/sim/hooks/queries/mcp.ts b/apps/sim/hooks/queries/mcp.ts index 8bbbedff3f8..21ce5202682 100644 --- a/apps/sim/hooks/queries/mcp.ts +++ b/apps/sim/hooks/queries/mcp.ts @@ -125,26 +125,31 @@ async function fetchMcpTools( } } +function isServerEligibleForDiscovery(server: McpServer, workspaceId: string): boolean { + return ( + server.enabled && + server.workspaceId === workspaceId && + (server.authType !== 'oauth' || server.connectionStatus === 'connected') + ) +} + /** * Workspace aggregate derived from N parallel per-server queries via * `useQueries`. One slow server cannot block the others. */ export function useMcpToolsQuery(workspaceId: string) { + const queryClient = useQueryClient() const { data: servers, isLoading: serversLoading } = useMcpServers(workspaceId) - // Skip disabled rows (would 404 → negative-cache), rows from a previous - // workspace (keepPreviousData on useMcpServers), and OAuth rows that need an - // explicit authorization/reconnect before discovery can succeed. + /** + * Skip disabled rows, rows retained from a previous workspace, and OAuth rows + * that require explicit authorization before discovery can succeed. + */ const serverIds = useMemo( () => servers ? servers - .filter( - (s) => - s.enabled && - s.workspaceId === workspaceId && - (s.authType !== 'oauth' || s.connectionStatus === 'connected') - ) + .filter((server) => isServerEligibleForDiscovery(server, workspaceId)) .map((s) => s.id) .sort() : [], @@ -154,8 +159,17 @@ export function useMcpToolsQuery(workspaceId: string) { const results = useQueries({ queries: serverIds.map((serverId) => ({ queryKey: mcpKeys.serverToolsList(workspaceId, serverId), - queryFn: ({ signal }: { signal?: AbortSignal }) => - fetchMcpTools(workspaceId, false, signal, serverId), + queryFn: async ({ signal }: { signal?: AbortSignal }) => { + try { + return await fetchMcpTools(workspaceId, false, signal, serverId) + } catch (error) { + await queryClient.invalidateQueries( + { queryKey: mcpKeys.serversList(workspaceId) }, + { cancelRefetch: false } + ) + throw error + } + }, enabled: !!workspaceId, retry: false, staleTime: MCP_SERVER_TOOLS_STALE_TIME, @@ -209,7 +223,9 @@ export function useForceRefreshMcpTools() { mutationFn: async (workspaceId: string) => { const allServers = queryClient.getQueryData(mcpKeys.serversList(workspaceId)) ?? [] - const servers = allServers.filter((s) => s.enabled && s.workspaceId === workspaceId) + const servers = allServers.filter((server) => + isServerEligibleForDiscovery(server, workspaceId) + ) const results = await Promise.allSettled( servers.map(async (server) => { const tools = await fetchMcpTools(workspaceId, true, undefined, server.id) diff --git a/apps/sim/lib/mcp/client.test.ts b/apps/sim/lib/mcp/client.test.ts index f2077ece1f2..de722343e48 100644 --- a/apps/sim/lib/mcp/client.test.ts +++ b/apps/sim/lib/mcp/client.test.ts @@ -141,6 +141,17 @@ describe('McpClient notification handler', () => { expect(mockSdkConnect).toHaveBeenCalledWith(expect.anything(), { timeout: 12_345 }) }) + it('normalizes invalid connection timeouts before calling the SDK', async () => { + const client = new McpClient({ + config: { ...createConfig(), timeout: -1 }, + securityPolicy: { requireConsent: false, auditLevel: 'basic' }, + }) + + await client.connect() + + expect(mockSdkConnect).toHaveBeenCalledWith(expect.anything(), { timeout: 30_000 }) + }) + it('logs connection diagnostics without header values', async () => { const client = new McpClient({ config: { @@ -188,13 +199,30 @@ describe('McpClient notification handler', () => { outcome: 'timeout', timeoutMs: 30_000, error: expect.objectContaining({ - message: 'MCP error -32001: Request timed out', + name: 'Error', }), }) ) expect(JSON.stringify(mockLogger.error.mock.calls)).not.toContain('do-not-log') }) + it('does not log opaque credentials echoed by MCP errors', async () => { + const secret = 'opaque-credential-without-a-known-prefix' + mockSdkConnect.mockRejectedValueOnce(new Error(`Upstream rejected ${secret}`)) + const client = new McpClient({ + config: { + ...createConfig(), + authType: 'headers', + headers: { 'X-Custom-Credential': secret }, + }, + securityPolicy: { requireConsent: false, auditLevel: 'basic' }, + }) + + await expect(client.connect()).rejects.toThrow('Upstream rejected') + + expect(JSON.stringify(mockLogger.error.mock.calls)).not.toContain(secret) + }) + it('passes configured headers for OAuth transports as well as header auth transports', () => { const authProvider = {} as unknown as NonNullable new McpClient({ diff --git a/apps/sim/lib/mcp/client.ts b/apps/sim/lib/mcp/client.ts index e3221093c67..f9e1485c824 100644 --- a/apps/sim/lib/mcp/client.ts +++ b/apps/sim/lib/mcp/client.ts @@ -113,7 +113,11 @@ export class McpClient { */ async connect(options: McpClientConnectOptions = {}): Promise { const startedAt = Date.now() - const timeoutMs = this.config.timeout ?? MCP_CLIENT_CONSTANTS.CLIENT_TIMEOUT + const configuredTimeout = this.config.timeout + const timeoutMs = + configuredTimeout !== undefined && Number.isFinite(configuredTimeout) && configuredTimeout > 0 + ? Math.min(Math.floor(configuredTimeout), getMaxExecutionTimeout()) + : MCP_CLIENT_CONSTANTS.CLIENT_TIMEOUT const headerNames = Object.keys(this.config.headers ?? {}).sort() const hasUnresolvedEnvRefs = [ this.config.url ?? '', @@ -167,21 +171,21 @@ export class McpClient { this.isConnected = false const errorMessage = getErrorMessage(error, 'Unknown error') const describedError = describeError(error) + const outcome = classifyConnectionOutcome(error) logger.error(`Failed to connect to MCP server ${this.config.name}`, { ...diagnostics, durationMs: Date.now() - startedAt, error: { - ...describedError, - message: sanitizeForLogging(describedError.message, 500), - causeChain: describedError.causeChain?.map((cause) => sanitizeForLogging(cause, 500)), - stack: - error instanceof Error && error.stack - ? sanitizeForLogging(error.stack.split('\n').slice(1).join('\n'), 2000) - : undefined, + name: sanitizeForLogging(describedError.name, 100), + code: describedError.code ? sanitizeForLogging(describedError.code, 100) : undefined, + errno: describedError.errno ? sanitizeForLogging(describedError.errno, 100) : undefined, + syscall: describedError.syscall + ? sanitizeForLogging(describedError.syscall, 100) + : undefined, }, - outcome: classifyConnectionOutcome(error), + outcome, }) - if (error instanceof McpOauthRedirectRequired || error instanceof UnauthorizedError) { + if (outcome === 'authorization_required') { this.connectionStatus.lastError = undefined throw error } diff --git a/apps/sim/lib/mcp/service.test.ts b/apps/sim/lib/mcp/service.test.ts index 3d84da348f7..c58050612b6 100644 --- a/apps/sim/lib/mcp/service.test.ts +++ b/apps/sim/lib/mcp/service.test.ts @@ -15,10 +15,12 @@ const { mockIsDomainAllowed, mockCacheAdapter, mockUpdateSet, + mockUpdateReturning, } = vi.hoisted(() => { const mockListTools = vi.fn() const mockConnect = vi.fn() const mockDisconnect = vi.fn() + const mockUpdateReturning = vi.fn().mockResolvedValue([{ id: 'server-1' }]) // In-memory cache adapter so the service never touches the real Redis the // local .env points at (unreachable in CI/sandbox → hangs). Honors TTL via // an expiry timestamp so negative-cache assertions behave like production. @@ -68,7 +70,10 @@ const { mockValidateDomain: vi.fn(), mockValidateSsrf: vi.fn(), mockIsDomainAllowed: vi.fn(() => true), - mockUpdateSet: vi.fn().mockReturnValue({ where: vi.fn().mockResolvedValue(undefined) }), + mockUpdateReturning, + mockUpdateSet: vi.fn().mockReturnValue({ + where: vi.fn().mockReturnValue({ returning: mockUpdateReturning }), + }), } }) @@ -174,6 +179,8 @@ describe('McpService.discoverTools per-server caching', () => { ) mockConnect.mockResolvedValue(undefined) mockDisconnect.mockResolvedValue(undefined) + mockUpdateReturning.mockReset() + mockUpdateReturning.mockResolvedValue([{ id: 'server-1' }]) // The McpService singleton holds cache state across imports. await mcpService.clearCache() }) @@ -424,4 +431,20 @@ describe('McpService.discoverTools per-server caching', () => { }) ) }) + + it('does not negative-cache a failure older than a successful discovery', async () => { + mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A')]) + mockListTools.mockRejectedValueOnce(new Error('Older request failed')) + mockUpdateReturning.mockResolvedValueOnce([]) + + await expect(mcpService.discoverServerTools(USER_ID, 'mcp-a', WORKSPACE_ID)).rejects.toThrow( + 'Older request failed' + ) + + mockListTools.mockResolvedValueOnce([tool('a1', 'mcp-a')]) + const tools = await mcpService.discoverServerTools(USER_ID, 'mcp-a', WORKSPACE_ID) + + expect(tools.map((tool) => tool.name)).toEqual(['a1']) + expect(mockListTools).toHaveBeenCalledTimes(2) + }) }) diff --git a/apps/sim/lib/mcp/service.ts b/apps/sim/lib/mcp/service.ts index b93d4c8a344..961e2ab27c8 100644 --- a/apps/sim/lib/mcp/service.ts +++ b/apps/sim/lib/mcp/service.ts @@ -5,7 +5,7 @@ import { mcpServers } from '@sim/db/schema' import { createLogger } from '@sim/logger' import { getErrorMessage } from '@sim/utils/errors' import { sleep } from '@sim/utils/helpers' -import { and, eq, isNotNull, isNull, ne, or } from 'drizzle-orm' +import { and, eq, isNull, lte, or } from 'drizzle-orm' import { isTest } from '@/lib/core/config/env-flags' import { generateRequestId } from '@/lib/core/utils/request' import { McpClient } from '@/lib/mcp/client' @@ -66,6 +66,10 @@ type DiscoveryOutcome = // exemption survives the getErrorMessage call. | { kind: 'error'; message: string; originalError: unknown } +type ServerStatusUpdate = + | { outcome: 'connected'; toolCount: number } + | { outcome: 'failed'; error: string; discoveryStartedAt?: Date } + function isOauthAuthorizationError(error: unknown): boolean { return error instanceof McpOauthAuthorizationRequiredError || error instanceof UnauthorizedError } @@ -315,21 +319,19 @@ class McpService { private async updateServerStatus( serverId: string, workspaceId: string, - success: boolean, - error?: string, - toolCount?: number - ): Promise { + update: ServerStatusUpdate + ): Promise { try { const now = new Date() - if (success) { + if (update.outcome === 'connected') { await db .update(mcpServers) .set({ connectionStatus: 'connected', lastConnected: now, lastError: null, - toolCount: toolCount ?? 0, + toolCount: update.toolCount, lastToolsRefresh: now, statusConfig: { consecutiveFailures: 0, @@ -338,7 +340,7 @@ class McpService { updatedAt: now, }) .where(eq(mcpServers.id, serverId)) - return + return true } const [currentServer] = await db @@ -365,24 +367,39 @@ class McpService { const newFailures = currentConfig.consecutiveFailures + 1 const isErrorState = newFailures >= MCP_CONSTANTS.MAX_CONSECUTIVE_FAILURES - await db + const updatedServers = await db .update(mcpServers) .set({ connectionStatus: isErrorState ? 'error' : 'disconnected', - lastError: error || 'Unknown error', + lastError: update.error || 'Unknown error', statusConfig: { consecutiveFailures: newFailures, lastSuccessfulDiscovery: currentConfig.lastSuccessfulDiscovery, }, updatedAt: now, }) - .where(eq(mcpServers.id, serverId)) + .where( + and( + eq(mcpServers.id, serverId), + eq(mcpServers.workspaceId, workspaceId), + isNull(mcpServers.deletedAt), + update.discoveryStartedAt + ? or( + isNull(mcpServers.lastConnected), + lte(mcpServers.lastConnected, update.discoveryStartedAt) + ) + : undefined + ) + ) + .returning({ id: mcpServers.id }) - if (isErrorState) { + if (isErrorState && updatedServers.length > 0) { logger.warn(`Server ${serverId} marked as error after ${newFailures} consecutive failures`) } + return updatedServers.length > 0 } catch (err) { logger.error(`Failed to update server status for ${serverId}:`, err) + return false } } @@ -409,9 +426,13 @@ class McpService { } } - private async markServerOauthPending(serverId: string, workspaceId: string): Promise { + private async markServerOauthPending( + serverId: string, + workspaceId: string, + discoveryStartedAt?: Date + ): Promise { try { - await db + const updatedServers = await db .update(mcpServers) .set({ connectionStatus: 'disconnected', @@ -423,15 +444,19 @@ class McpService { eq(mcpServers.id, serverId), eq(mcpServers.workspaceId, workspaceId), isNull(mcpServers.deletedAt), - or( - isNull(mcpServers.connectionStatus), - ne(mcpServers.connectionStatus, 'disconnected'), - isNotNull(mcpServers.lastError) - ) + discoveryStartedAt + ? or( + isNull(mcpServers.lastConnected), + lte(mcpServers.lastConnected, discoveryStartedAt) + ) + : undefined ) ) + .returning({ id: mcpServers.id }) + return updatedServers.length > 0 } catch (error) { logger.warn(`Failed to mark OAuth server ${serverId} disconnected:`, error) + return false } } @@ -458,6 +483,7 @@ class McpService { forceRefresh = false ): Promise { const requestId = generateRequestId() + const discoveryStartedAt = new Date() try { logger.info(`[${requestId}] Discovering MCP tools for workspace ${workspaceId}`) @@ -542,7 +568,10 @@ class McpService { fetchedCount++ allTools.push(...outcome.tools) deferredSideEffects.push( - this.updateServerStatus(server.id, workspaceId, true, undefined, outcome.tools.length) + this.updateServerStatus(server.id, workspaceId, { + outcome: 'connected', + toolCount: outcome.tools.length, + }) ) cacheWrites.push( this.cacheAdapter @@ -561,7 +590,11 @@ class McpService { if (outcome.kind === 'oauth-pending') { // Mark disconnected so the UI surfaces the re-auth button. logger.info(`[${requestId}] Skipping server ${server.name}: OAuth authorization pending`) - deferredSideEffects.push(this.markServerOauthPending(server.id, workspaceId)) + deferredSideEffects.push( + this.markServerOauthPending(server.id, workspaceId, discoveryStartedAt).then( + () => undefined + ) + ) return } if (outcome.kind === 'unhealthy') { @@ -574,13 +607,21 @@ class McpService { `[${requestId}] Failed to discover tools from server ${server.name}: ${outcome.message}` ) deferredSideEffects.push( - this.updateServerStatus(server.id, workspaceId, false, outcome.message), - this.markServerUnhealthy(workspaceId, server.id, outcome.originalError), - this.cacheAdapter - .delete(serverCacheKey(workspaceId, server.id)) - .catch((err) => - logger.warn(`[${requestId}] Cache delete failed for ${server.name}:`, err) - ) + this.updateServerStatus(server.id, workspaceId, { + outcome: 'failed', + error: outcome.message, + discoveryStartedAt, + }).then(async (statusApplied) => { + if (!statusApplied) return + await Promise.allSettled([ + this.markServerUnhealthy(workspaceId, server.id, outcome.originalError), + this.cacheAdapter + .delete(serverCacheKey(workspaceId, server.id)) + .catch((err) => + logger.warn(`[${requestId}] Cache delete failed for ${server.name}:`, err) + ), + ]) + }) ) }) @@ -649,6 +690,7 @@ class McpService { forceRefresh: boolean ): Promise { const requestId = generateRequestId() + const discoveryStartedAt = new Date() const maxRetries = 2 if (!forceRefresh) { @@ -698,7 +740,10 @@ class McpService { logger.warn(`[${requestId}] Cache write failed for ${config.name}:`, err) ), this.clearServerFailure(workspaceId, serverId), - this.updateServerStatus(serverId, workspaceId, true, undefined, tools.length), + this.updateServerStatus(serverId, workspaceId, { + outcome: 'connected', + toolCount: tools.length, + }), ]) return tools } finally { @@ -714,23 +759,23 @@ class McpService { continue } // Drop positive cache so a follow-up doesn't return stale tools. - const statusUpdate = isOauthAuthorizationError(error) - ? this.markServerOauthPending(serverId, workspaceId) - : this.updateServerStatus( - serverId, - workspaceId, - false, - getErrorMessage(error, 'Connection failed') - ) - await Promise.allSettled([ - this.cacheAdapter - .delete(serverCacheKey(workspaceId, serverId)) - .catch((err) => - logger.warn(`[${requestId}] Cache delete failed for ${serverId}:`, err) - ), - this.markServerUnhealthy(workspaceId, serverId, error), - statusUpdate, - ]) + const statusApplied = isOauthAuthorizationError(error) + ? await this.markServerOauthPending(serverId, workspaceId, discoveryStartedAt) + : await this.updateServerStatus(serverId, workspaceId, { + outcome: 'failed', + error: getErrorMessage(error, 'Connection failed'), + discoveryStartedAt, + }) + if (statusApplied) { + await Promise.allSettled([ + this.cacheAdapter + .delete(serverCacheKey(workspaceId, serverId)) + .catch((err) => + logger.warn(`[${requestId}] Cache delete failed for ${serverId}:`, err) + ), + this.markServerUnhealthy(workspaceId, serverId, error), + ]) + } throw error } }