fix: Honor AbortSignal on Actor-MCP calls and hash-dedupe proxy names - #1185
Conversation
Remote Actor-MCP callTool ignored cancel/disconnect, and bare truncation could collide over-length proxied tool names.
fc78876 to
cd5889c
Compare
There was a problem hiding this comment.
🟢 Ready to approve
The changes are minimal, match stated invariants, and are backed by focused unit/contract tests for both abort propagation and proxy name deduping.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR fixes two correctness issues in the MCP proxying path: (1) Actor-MCP tool calls now receive the request/task AbortSignal so cancellations can stop remote work before the 120s timeout, and (2) proxied Actor-MCP tool names now follow the repo’s “cap + hash-suffix” invariant to avoid truncation collisions.
Changes:
- Forward
AbortSignal(and timeout) into remoteclient.callTool(...)options for Actor-MCP calls (bothdispatchToolCallandcall-actorpassthrough). - Hash-suffix over-length proxied MCP tool names to prevent collisions caused by bare truncation.
- Add/extend unit tests to assert signal forwarding behavior and proxy name hashing/deduping.
File summaries
| File | Description |
|---|---|
| tests/unit/tools.call_actor_common.test.ts | Adds unit coverage for handleMcpToolCall() signal/timeout forwarding and abort handling. |
| tests/unit/mcp.server.tool_call_contracts.test.ts | Adds contract test ensuring tools/call forwards abort signal into Actor-MCP callTool options. |
| tests/unit/mcp.proxy.test.ts | New tests for stable server IDs and hash-suffixed proxy tool naming under the length cap. |
| src/tools/actors/call_actor.ts | Passes signal + timeout into remote Actor-MCP callTool and returns aborted early when applicable. |
| src/mcp/tool_dispatch.ts | Forwards signal into Actor-MCP callTool options (sync + task mode). |
| src/mcp/proxy.ts | Implements hash-suffixing for over-length proxy tool names and exports getProxyMCPServerToolName. |
| src/mcp/AGENTS.md | Updates docs to reflect proxy tool naming invariant and new helper export. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 0
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
Nice work @Ayush7614 - this found two real problems, and the tool-name fix is straightforwardly correct.
The abort forwarding is right too, and it's a real improvement: cancel now returns immediately instead of holding the call for up to 120s. One piece is missing, and it isn't visible from the client side - the explicit notifications/cancelled never reaches the remote, because client.close() in the finally aborts the same AbortController the notification rides on. I caught it by standing up a live remote and logging what arrived. The SDK only documents signal as raising an AbortError locally, so you got exactly what's specified.
Two comments below with the fix - one macrotask yield in each catch. Verified against a real remote: cancellation delivered, remote handler aborts, no added latency, unit suite green. This same race silently breaks the pre-existing 120s timeout path too, so your PR plus this patch fixes something older than your change.
One thing to note in the description: passing EXTERNAL_TOOL_CALL_TIMEOUT_MSEC in handleMcpToolCall moves that call from the SDK default 60s to 120s. Parity with the dispatch path makes sense, but it doubles worst-case latency on that route, so it's worth stating.
Two optional nits as suggestions, plus one on a test. Take or leave them.
Process note, and the timing is on us: we published new contribution guidelines a few days after you opened this - issue-first, AI disclosure, evidence. Not applying them retroactively. Worth a read before the next one: CONTRIBUTING.md
Yield a macrotask in the abort catch so notifications/cancelled can leave the wire before finally close() aborts the transport controller.
3db2d98 to
a0a108c
Compare
|
Thanks @vojtechj-apify — addressed in a0a108c. Required fix: macrotask yield ( Nits taken:
PR description: noted that Unit suite still green. |
vojtechj-apify
left a comment
vojtechj-apify
left a comment
There was a problem hiding this comment.
Thanks for the fix, looks all good now!
On abort the ACTOR_MCP branch fell through its generic catch, producing an
error result body, failure_category INTERNAL_ERROR and failure_detail, and
a failure log — at error level when the abort landed while connecting, where
callTool throws a bare DOMException that no logHttpError branch matches.
Return result = {} and break instead, matching the ACTOR branch. tool_status
was already ABORTED via buildExecutionDiagnostics; what changes is the body,
the failure_* fields and the log. Reachable since #1185 started passing the
abort signal into client.callTool().
## Why Cancelling an Actor-MCP tool call is reported as a failure. Probed against a local MCP server: an error result body and `failure_category: INTERNAL_ERROR`, plus a failure log. (`tool_status` was already `ABORTED` — `buildExecutionDiagnostics` derives it from `isAborted` — so the status was never the problem.) An abort mid-call throws `McpError(-32001)`, which is in `SOFT_MCP_ERROR_CODES`, so `log.softFail`. An abort that lands while `connectMCPClient` is in flight is worse: `connectMCPClient` itself never throws on abort, but the subsequent `client.callTool` calls `signal.throwIfAborted()` and raises a bare `DOMException` (`AbortError`, `code: 20`) that no `logHttpError` branch matches — 20 is outside 100–600 and −32768..−32000, and "This operation was aborted" fits neither transient pattern — so it reaches the final `log.error`. Alert level, on a user pressing cancel. Reachable since #1185 started passing the abort signal into `client.callTool()`. In `@apify/actors-mcp-server 0.14.2-beta.2`; `latest` is still `0.14.1`. ## What changed Before: an aborted `ACTOR_MCP` call fell through the generic catch to `buildExecutionDiagnostics` and `logHttpError`. Now: it returns `result = {}` and breaks, matching the `ACTOR` branch. `tool_status` stays `ABORTED`; the error body, the `failure_*` fields and the log go away. Also updated two comments in the same file that described the catch as always producing a soft-fail `isError` body. ## Notes for reviewers The one decision here is logging nothing on a cancel. That matches the `ACTOR` branch, but if you'd rather a cancellation left a debug trace, that's a one-line change. Known trade-off: the guard keys on `signal.aborted`, not on the error being abort-caused. So a genuine remote failure that races with a cancel is now dropped with no log, where before it was misfiled as `INTERNAL_ERROR` but at least logged. Keying on the signal is the existing convention here (`handleMcpToolCall` and `buildExecutionDiagnostics` both do it), so I left it alone — say the word if you'd rather narrow it to abort-shaped errors. Telemetry shifts: `failure_category` and `failure_detail` are no longer emitted for these. Anything grouping cancelled Actor-MCP calls under `INTERNAL_ERROR` will see that bucket empty out — intended, but worth telling whoever owns those charts. There is no cancel/abort value in `FAILURE_CATEGORY`, and the three `ABORTED` sites in `task_execution.ts` already emit actor fields only, so unset is the existing convention. Task mode is unaffected: the cancel-watcher signal only aborts after the store shows the task cancelled, so `skipIfTaskCancelled` short-circuits before the `{}` could be stored. No `apify-mcp-server-internal` surface touched — no `internals.js` exports, `_meta`, `structuredContent`, or `?ui=`/`?payment=` parsing. ## Proof it works Probe against a real SDK `Server` with a hanging tool, cancelled mid-call: ``` before: toolStatus=ABORTED errorBody=true failure_category=INTERNAL_ERROR after: toolStatus=ABORTED errorBody=false failure_category=<unset> ``` New unit test asserts the aborted result carries no error body and that `logHttpError` is not called; both assertions fail independently when the guard is removed. Local gates: `type-check`, `lint`, `format`, `check:agents`, 1268 unit tests. Found and fixed with Claude Code while reviewing #1185; a second model did an adversarial pass over the diff and description, which is where the title correction and the stale comments came from.
Summary
Notes
Why
Actor and internal tools already honor abort. Actor-MCP only checked signal.aborted in the catch after failure — the SDK call never received signal. Proxy naming documented the collision risk and violated the capped + hash-deduped tool-name invariant in src/mcp/AGENTS.md.
Test plan