fix: Honor AbortSignal on Actor-MCP calls and hash-dedupe proxy names

Ayush7614 · Ayush7614 · commit cd5889cb0913 · 2026-07-30T21:15:24.000+05:30
Remote Actor-MCP callTool ignored cancel/disconnect, and bare truncation could collide over-length proxied tool names.
diff --git a/src/mcp/AGENTS.md b/src/mcp/AGENTS.md
@@ -34,7 +34,7 @@ Two MCP protocol revisions are served, each by its own adapter:
 - `tool_call_engine.ts` — shared `tools/call` orchestration. `prepareToolCall()` handles
   preparation; `executeSyncToolCall()` runs synchronous calls.
 - `client.ts` — `connectMCPClient(url, token)`: transport negotiation.
-- `proxy.ts` — MCP-in-MCP: `getMCPServerID(url)`.
+- `proxy.ts` — MCP-in-MCP: `getMCPServerID(url)`, `getProxyMCPServerToolName(url, toolName)`.
 - `actors.ts` — `getActorMCPServerPath()`: parses an Actor's `webServerMcpPath`.
 - `utils.ts` — `processParamsGetTools()`: turns `?actors=` URL params into tools.
 - `tool_call_error_mapper.ts` — shared tool-call error classification.
@@ -61,8 +61,9 @@ Two MCP protocol revisions are served, each by its own adapter:
   each other.
 - **Tool names: capped + hash-deduped.** Names are capped at `MAX_TOOL_NAME_LENGTH`;
   over-length or colliding names get a `TOOL_NAME_HASH_LENGTH` hash suffix so the
-  exposed set stays unique within the limit (the hashing is in `../tools/actor_tool_naming.ts`).
-  Never widen the cap — downstream clients depend on it.
+  exposed set stays unique within the limit (Actor tools: `../tools/actor_tool_naming.ts`;
+  proxied Actor-MCP tools: `proxy.ts` `getProxyMCPServerToolName`). Never widen the
+  cap — downstream clients depend on it.
 - **Proxy server IDs are keyed by URL, not Actor ID.** `getMCPServerID(url)` is
   `sha256(url)` sliced to `SERVER_ID_LENGTH`. One Actor can expose both an SSE and a
   streamable endpoint; keying by URL keeps those distinct. Keying by Actor ID would
diff --git a/src/mcp/proxy.ts b/src/mcp/proxy.ts
@@ -6,7 +6,7 @@ import { fixedAjvCompile } from '../tools/actor_input_schema.js';
 import type { ActorMcpTool, ToolEntry } from '../types.js';
 import { TOOL_TYPE } from '../types.js';
 import { ajv } from '../utils/ajv.js';
-import { MAX_TOOL_NAME_LENGTH, SERVER_ID_LENGTH } from './const.js';
+import { MAX_TOOL_NAME_LENGTH, SERVER_ID_LENGTH, TOOL_NAME_HASH_LENGTH } from './const.js';
 
 /**
  * Generates a unique server ID by hashing the URL.
@@ -20,14 +20,20 @@ export function getMCPServerID(url: string): string {
 }
 
 /**
- * Prefixes the tool name with the server ID hash and truncates to MAX_TOOL_NAME_LENGTH.
- * Truncation can in theory collide two different origin tool names.
+ * Prefixes the tool name with the server ID hash. Over-length names get a hash suffix
+ * (same pattern as actor tool names) so bare truncation cannot collide two different
+ * origin tool names into one exposed name.
  */
-function getProxyMCPServerToolName(url: string, toolName: string): string {
+export function getProxyMCPServerToolName(url: string, toolName: string): string {
     const prefix = getMCPServerID(url);
-
     const fullName = `${prefix}-${toolName}`;
-    return fullName.slice(0, MAX_TOOL_NAME_LENGTH);
+
+    if (fullName.length <= MAX_TOOL_NAME_LENGTH) {
+        return fullName;
+    }
+
+    const hash = createHash('sha256').update(fullName).digest('hex').slice(0, TOOL_NAME_HASH_LENGTH);
+    return `${fullName.slice(0, MAX_TOOL_NAME_LENGTH - TOOL_NAME_HASH_LENGTH - 1)}-${hash}`;
 }
 
 export async function getMCPServerTools(actorID: string, client: Client, serverUrl: string): Promise<ToolEntry[]> {
diff --git a/src/mcp/tool_dispatch.ts b/src/mcp/tool_dispatch.ts
@@ -181,6 +181,10 @@ export async function dispatchToolCall(params: {
                     CallToolResultSchema,
                     {
                         timeout: EXTERNAL_TOOL_CALL_TIMEOUT_MSEC,
+                        // Same abort source as ACTOR/INTERNAL branches: request signal for sync,
+                        // cancel-watcher signal for tasks. Without this, cancel/disconnect cannot
+                        // stop the remote call before EXTERNAL_TOOL_CALL_TIMEOUT_MSEC.
+                        signal,
                     },
                 );
 
diff --git a/src/tools/actors/call_actor.ts b/src/tools/actors/call_actor.ts
@@ -1,5 +1,6 @@
 import type { Client } from '@modelcontextprotocol/sdk/client/index.js';
 import type { ContentBlock } from '@modelcontextprotocol/sdk/types.js';
+import { CallToolResultSchema } from '@modelcontextprotocol/sdk/types.js';
 import dedent from 'dedent';
 import { z } from 'zod';
 
@@ -15,6 +16,7 @@ import {
 } from '../../const.js';
 import { ACTOR_LOAD_ERROR_KIND, ActorLoadError } from '../../errors.js';
 import { connectMCPClient } from '../../mcp/client.js';
+import { EXTERNAL_TOOL_CALL_TIMEOUT_MSEC } from '../../mcp/const.js';
 import type { PaymentProvider } from '../../payments/types.js';
 import type { ActorInfo, ApifyToken, InternalToolArgs, ToolEntry, ToolInputSchema } from '../../types.js';
 import { TOOL_TYPE } from '../../types.js';
@@ -324,8 +326,10 @@ export async function handleMcpToolCall(params: {
     mcpServerUrl: string | false;
     apifyToken: string;
     mcpSessionId?: string;
+    signal: AbortSignal;
 }): Promise<ToolResponse | null> {
-    const { baseActorName, mcpToolName, input, isActorMcpServer, mcpServerUrl, apifyToken, mcpSessionId } = params;
+    const { baseActorName, mcpToolName, input, isActorMcpServer, mcpServerUrl, apifyToken, mcpSessionId, signal } =
+        params;
 
     if (!isActorMcpServer) {
         return respondServerError(`Actor '${baseActorName}' is not an MCP server.`);
@@ -337,17 +341,32 @@ export async function handleMcpToolCall(params: {
         );
     }
 
+    if (signal.aborted) {
+        return respondAborted();
+    }
+
     let client: Client | null = null;
     try {
         client = await connectMCPClient(mcpServerUrl as string, apifyToken, mcpSessionId);
         if (!client) {
             return respondServerError(`Failed to connect to MCP server ${mcpServerUrl}`);
         }
 
-        const result = await client.callTool({
-            name: mcpToolName,
-            arguments: input,
-        });
+        if (signal.aborted) {
+            return respondAborted();
+        }
+
+        const result = await client.callTool(
+            {
+                name: mcpToolName,
+                arguments: input,
+            },
+            CallToolResultSchema,
+            {
+                timeout: EXTERNAL_TOOL_CALL_TIMEOUT_MSEC,
+                signal,
+            },
+        );
 
         // `call-actor` declares `actorRunOutputSchema`, so MCP SDK ≥ 1.11.4 rejects any response
         // without `structuredContent` (unless `isError: true`) with -32600. The pass-through has no
@@ -369,6 +388,9 @@ export async function handleMcpToolCall(params: {
             },
         });
     } catch (error) {
+        if (signal.aborted) {
+            return respondAborted();
+        }
         logHttpError(error, `Failed to call MCP tool '${mcpToolName}' on Actor '${baseActorName}'`, {
             actorName: baseActorName,
             toolName: mcpToolName,
@@ -529,6 +551,7 @@ export async function callActorPreExecute(
             mcpServerUrl: mcpServerUrlOrFalse,
             apifyToken,
             mcpSessionId,
+            signal: toolArgs.signal,
         });
         if (mcpResult) {
             return { earlyResponse: mcpResult };
diff --git a/tests/unit/mcp.proxy.test.ts b/tests/unit/mcp.proxy.test.ts
@@ -0,0 +1,51 @@
+import { createHash } from 'node:crypto';
+
+import { describe, expect, it } from 'vitest';
+
+import { MAX_TOOL_NAME_LENGTH, SERVER_ID_LENGTH, TOOL_NAME_HASH_LENGTH } from '../../src/mcp/const.js';
+import { getMCPServerID, getProxyMCPServerToolName } from '../../src/mcp/proxy.js';
+
+describe('getMCPServerID()', () => {
+    it('returns a stable SERVER_ID_LENGTH hex prefix of sha256(url)', () => {
+        const url = 'https://example.com/mcp';
+        const expected = createHash('sha256').update(url).digest('hex').slice(0, SERVER_ID_LENGTH);
+        expect(getMCPServerID(url)).toBe(expected);
+        expect(getMCPServerID(url)).toBe(getMCPServerID(url));
+    });
+
+    it('keys by URL so SSE and streamable endpoints stay distinct', () => {
+        expect(getMCPServerID('https://actor.example/sse')).not.toBe(getMCPServerID('https://actor.example/mcp'));
+    });
+});
+
+describe('getProxyMCPServerToolName()', () => {
+    const url = 'https://example.com/mcp';
+
+    it('returns prefix-toolName when under the length cap', () => {
+        const name = getProxyMCPServerToolName(url, 'list-items');
+        expect(name).toBe(`${getMCPServerID(url)}-list-items`);
+        expect(name.length).toBeLessThanOrEqual(MAX_TOOL_NAME_LENGTH);
+    });
+
+    it('hash-suffixes over-length names instead of bare truncation', () => {
+        const longTool = `very-long-origin-tool-name-${'x'.repeat(80)}`;
+        const fullName = `${getMCPServerID(url)}-${longTool}`;
+        const hash = createHash('sha256').update(fullName).digest('hex').slice(0, TOOL_NAME_HASH_LENGTH);
+        const name = getProxyMCPServerToolName(url, longTool);
+
+        expect(name.length).toBe(MAX_TOOL_NAME_LENGTH);
+        expect(name.endsWith(`-${hash}`)).toBe(true);
+        // Bare slice would drop the distinguishing suffix and collide; hash must survive.
+        expect(name).not.toBe(fullName.slice(0, MAX_TOOL_NAME_LENGTH));
+    });
+
+    it('keeps two over-length origin names distinct after capping', () => {
+        const sharedPrefix = `shared-prefix-${'y'.repeat(80)}`;
+        const a = getProxyMCPServerToolName(url, `${sharedPrefix}-alpha`);
+        const b = getProxyMCPServerToolName(url, `${sharedPrefix}-beta`);
+
+        expect(a).not.toBe(b);
+        expect(a.length).toBe(MAX_TOOL_NAME_LENGTH);
+        expect(b.length).toBe(MAX_TOOL_NAME_LENGTH);
+    });
+});
diff --git a/tests/unit/mcp.server.tool_call_contracts.test.ts b/tests/unit/mcp.server.tool_call_contracts.test.ts
@@ -702,6 +702,37 @@ describe('ACTOR_MCP remote-McpError containment (sync tools/call catch)', () =>
         expect(properties.failure_category).toBe(FAILURE_CATEGORY.INVALID_INPUT);
     });
 
+    it('forwards the abort signal into the remote Actor-MCP callTool options', async () => {
+        // Without `signal` in callTool options, cancel/disconnect cannot stop the remote call before
+        // EXTERNAL_TOOL_CALL_TIMEOUT_MSEC — ACTOR/INTERNAL already abort; ACTOR_MCP must match.
+        await withServer(async (server) => {
+            silenceLogs();
+            const callTool = vi.fn().mockResolvedValue({ content: [{ type: 'text', text: 'ok' }] });
+            const stubClient = {
+                callTool,
+                close: vi.fn().mockResolvedValue(undefined),
+                setNotificationHandler: vi.fn(),
+            } as unknown as Client;
+            vi.spyOn(mcpClient, 'connectMCPClient').mockResolvedValue(stubClient);
+
+            const controller = new AbortController();
+            server.upsertTools([makeActorMcpTool()]);
+            const handler = getRequestHandler(server, 'tools/call');
+            await handler(
+                {
+                    method: 'tools/call',
+                    params: { name: 'test-actor-mcp-tool', arguments: {}, _meta: { mcpSessionId: 's1' } },
+                },
+                { signal: controller.signal, sendNotification: vi.fn() },
+            );
+
+            expect(callTool).toHaveBeenCalledTimes(1);
+            const options = callTool.mock.calls[0][2] as { signal?: AbortSignal; timeout?: number };
+            expect(options.signal).toBe(controller.signal);
+            expect(options.timeout).toBeGreaterThan(0);
+        });
+    });
+
     it('re-throws an escaped McpError as a JSON-RPC error, not an isError tool result', async () => {
         // Escaped McpErrors must remain JSON-RPC errors, including 402-coded errors.
         await withServer(async (server) => {
diff --git a/tests/unit/tools.call_actor_common.test.ts b/tests/unit/tools.call_actor_common.test.ts
@@ -1,3 +1,4 @@
+import type { Client } from '@modelcontextprotocol/sdk/client/index.js';
 import { ApifyApiError } from 'apify-client';
 import type { AxiosResponse } from 'axios';
 import { beforeEach, describe, expect, it, vi } from 'vitest';
@@ -8,11 +9,14 @@ import {
     HELPER_TOOLS,
     TOOL_STATUS,
 } from '../../src/const.js';
+import * as mcpClient from '../../src/mcp/client.js';
+import { EXTERNAL_TOOL_CALL_TIMEOUT_MSEC } from '../../src/mcp/const.js';
 import {
     buildCallActorAppsDescription,
     buildCallActorDescription,
     buildCallActorErrorResponse,
     callActorArgs,
+    handleMcpToolCall,
     resolveAndValidateActor,
 } from '../../src/tools/actors/call_actor.js';
 import type { InternalToolArgs, ToolEntry } from '../../src/types.js';
@@ -331,4 +335,80 @@ describe('call_actor_common', () => {
             });
         });
     });
+
+    describe('handleMcpToolCall()', () => {
+        beforeEach(() => {
+            vi.restoreAllMocks();
+        });
+
+        it('forwards signal and timeout into client.callTool options', async () => {
+            const callTool = vi.fn().mockResolvedValue({
+                content: [{ type: 'text', text: 'remote ok' }],
+                isError: false,
+            });
+            vi.spyOn(mcpClient, 'connectMCPClient').mockResolvedValue({
+                callTool,
+                close: vi.fn().mockResolvedValue(undefined),
+            } as unknown as Client);
+
+            const controller = new AbortController();
+            const result = await handleMcpToolCall({
+                baseActorName: 'apify/mcp-demo',
+                mcpToolName: 'search',
+                input: { q: 'x' },
+                isActorMcpServer: true,
+                mcpServerUrl: 'https://example.invalid/mcp',
+                apifyToken: 'token',
+                signal: controller.signal,
+            });
+
+            expect(result?.isError).not.toBe(true);
+            expect(callTool).toHaveBeenCalledTimes(1);
+            const options = callTool.mock.calls[0][2] as { signal?: AbortSignal; timeout?: number };
+            expect(options.signal).toBe(controller.signal);
+            expect(options.timeout).toBe(EXTERNAL_TOOL_CALL_TIMEOUT_MSEC);
+        });
+
+        it('returns aborted when the signal is already aborted before the remote call', async () => {
+            const connectSpy = vi.spyOn(mcpClient, 'connectMCPClient');
+            const controller = new AbortController();
+            controller.abort();
+
+            const result = await handleMcpToolCall({
+                baseActorName: 'apify/mcp-demo',
+                mcpToolName: 'search',
+                input: { q: 'x' },
+                isActorMcpServer: true,
+                mcpServerUrl: 'https://example.invalid/mcp',
+                apifyToken: 'token',
+                signal: controller.signal,
+            });
+
+            expect(result).toEqual({});
+            expect(connectSpy).not.toHaveBeenCalled();
+        });
+
+        it('returns aborted when the remote call rejects after the signal aborts', async () => {
+            const controller = new AbortController();
+            vi.spyOn(mcpClient, 'connectMCPClient').mockResolvedValue({
+                callTool: vi.fn().mockImplementation(async () => {
+                    controller.abort();
+                    throw new Error('aborted');
+                }),
+                close: vi.fn().mockResolvedValue(undefined),
+            } as unknown as Client);
+
+            const result = await handleMcpToolCall({
+                baseActorName: 'apify/mcp-demo',
+                mcpToolName: 'search',
+                input: { q: 'x' },
+                isActorMcpServer: true,
+                mcpServerUrl: 'https://example.invalid/mcp',
+                apifyToken: 'token',
+                signal: controller.signal,
+            });
+
+            expect(result).toEqual({});
+        });
+    });
 });
