-
Notifications
You must be signed in to change notification settings - Fork 78
fix: Actorized MCP servers have 30 seconds timeout to connect #272
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.
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
Changes from 4 commits
5f18d34
4591042
693de47
22f4c9d
8ad0741
2ca73dd
752021e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,24 +4,68 @@ import { StreamableHTTPClientTransport } from '@modelcontextprotocol/sdk/client/ | |
|
|
||
| import log from '@apify/log'; | ||
|
|
||
| import { ACTORIZED_MCP_CONNECTION_TIMEOUT_MSEC } from './const.js'; | ||
| import { getMCPServerID } from './utils.js'; | ||
|
|
||
| class TimeoutError extends Error { | ||
| override readonly name = 'TimeoutError'; | ||
| } | ||
|
|
||
| /** | ||
| * Creates and connects a ModelContextProtocol client. | ||
| * First tries streamable HTTP transport, then falls back to SSE transport. | ||
| */ | ||
| export async function connectMCPClient( | ||
| url: string, token: string, | ||
| ): Promise<Client> { | ||
| ): Promise<Client | null> { | ||
| let client: Client; | ||
| try { | ||
| return await createMCPStreamableClient(url, token); | ||
| } catch { | ||
| client = await createMCPStreamableClient(url, token); | ||
| return client; | ||
| } catch (error) { | ||
| // If streamable HTTP transport fails on not timeout error, continue with SSE transport | ||
| if (error instanceof TimeoutError) { | ||
| log.warning('Connection to MCP server using streamable HTTP transport timed out', { url }); | ||
| return null; | ||
| } | ||
|
|
||
| // If streamable HTTP transport fails, fall back to SSE transport | ||
| log.debug('Streamable HTTP transport failed, falling back to SSE transport', { | ||
| url, | ||
| }); | ||
| return await createMCPSSEClient(url, token); | ||
| } | ||
|
|
||
| try { | ||
| client = await createMCPSSEClient(url, token); | ||
| return client; | ||
| } catch (error) { | ||
| if (error instanceof TimeoutError) { | ||
| log.warning('Connection to MCP server using SSE transport timed out', { url }); | ||
| return null; | ||
| } | ||
|
|
||
| log.error('Failed to connect to MCP server using SSE transport', { cause: error }); | ||
| throw error; | ||
|
Comment on lines
+34
to
+45
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit/question: Why is the try block here? We are just catching and then re-throwing. I would remove the try block for the SSE.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I throw exception, connection will not be opened. |
||
| } | ||
| } | ||
|
|
||
| async function withTimeout<T>(millis: number, promise: Promise<T>): Promise<T> { | ||
| let timeoutPid: NodeJS.Timeout; | ||
| const timeout = new Promise<never>((_resolve, reject) => { | ||
| timeoutPid = setTimeout( | ||
| () => reject(new TimeoutError(`Timed out after ${millis} ms.`)), | ||
| millis, | ||
| ); | ||
| }); | ||
|
|
||
| return Promise.race([ | ||
| promise, | ||
| timeout, | ||
| ]).finally(() => { | ||
| if (timeoutPid) { | ||
| clearTimeout(timeoutPid); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -56,7 +100,7 @@ async function createMCPSSEClient( | |
| version: '1.0.0', | ||
| }); | ||
|
|
||
| await client.connect(transport); | ||
| await withTimeout(ACTORIZED_MCP_CONNECTION_TIMEOUT_MSEC, client.connect(transport)); | ||
|
|
||
| return client; | ||
| } | ||
|
|
@@ -82,7 +126,7 @@ async function createMCPStreamableClient( | |
| version: '1.0.0', | ||
| }); | ||
|
|
||
| await client.connect(transport); | ||
| await withTimeout(ACTORIZED_MCP_CONNECTION_TIMEOUT_MSEC, client.connect(transport)); | ||
|
|
||
| return client; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -206,21 +206,22 @@ async function getMCPServersAsTools( | |
| /** | ||
| * This is case for the Skyfire request without any Apify token, we do not support | ||
| * standby Actors in this case so we can skip MCP servers since they would fail anyway (they are standby Actors). | ||
| */ | ||
| */ | ||
| if (apifyToken === null || apifyToken === undefined) { | ||
| return []; | ||
| } | ||
|
|
||
| const actorsMCPServerTools: ToolEntry[] = []; | ||
| for (const actorInfo of actorsInfo) { | ||
| // Process all actors in parallel | ||
| const actorToolPromises = actorsInfo.map(async (actorInfo) => { | ||
| const actorId = actorInfo.actorDefinitionPruned.id; | ||
| if (!actorInfo.webServerMcpPath) { | ||
| log.warning('Actor does not have a web server MCP path, skipping', { | ||
| actorFullName: actorInfo.actorDefinitionPruned.actorFullName, | ||
| actorId, | ||
| }); | ||
| continue; | ||
| return []; | ||
| } | ||
|
|
||
| const mcpServerUrl = await getActorMCPServerURL( | ||
| actorInfo.actorDefinitionPruned.id, // Real ID of the Actor | ||
| actorInfo.webServerMcpPath, | ||
|
|
@@ -231,17 +232,25 @@ async function getMCPServersAsTools( | |
| mcpServerUrl, | ||
| }); | ||
|
|
||
| let client: Client | undefined; | ||
| let client: Client | null = null; | ||
| try { | ||
| client = await connectMCPClient(mcpServerUrl, apifyToken); | ||
| if (!client) { | ||
| // Skip this Actor, connectMCPClient will log the error | ||
| return []; | ||
| } | ||
| const serverTools = await getMCPServerTools(actorId, client, mcpServerUrl); | ||
| actorsMCPServerTools.push(...serverTools); | ||
| return serverTools; | ||
| } finally { | ||
| if (client) await client.close(); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| // Wait for all actors to be processed in parallel | ||
| const actorToolsArrays = await Promise.all(actorToolPromises); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. notice: for the most use cases it should be fine, but we should be aware of rate limits
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't find any rate limiting docs in the public docs on internal Notion. What type of limits do you mean? |
||
|
|
||
| return actorsMCPServerTools; | ||
| // Flatten the arrays of tools | ||
| return actorToolsArrays.flat(); | ||
| } | ||
|
|
||
| export async function getActorsAsTools( | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.