Skip to content

Commit 6814a74

Browse files
committed
Bound MCP tool calls with their own watchdog timeout
A wedged MCP server previously hung a tool call (and the stall watchdog treats an in-flight tool as activity, so the turn) forever, since the generic tool watchdog no longer arms by default. mcp__* calls now arm unconditionally through resolveToolExecutionTimeoutMs, defaulting to 5 minutes (settings.mcp.timeoutMs to override, capped by tools.maxTimeoutMs), and time out with a normal model-reactable tool error instead of hanging. Investigated the MCP client for a concurrency deadlock: our client.ts and the vendored SDK's Protocol.request() multiplex requests by JSON-RPC id with no serial queue or mutex, so parallel calls to one server should not deadlock each other. Live forensics for this issue showed multi-minute MCP calls that later completed, consistent with a slow server rather than a client-side deadlock. Fixes CL-6895 https://linear.app/abklabs/issue/CL-6895
1 parent 04b767b commit 6814a74

7 files changed

Lines changed: 175 additions & 13 deletions

File tree

docs/ARCHITECTURE.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -378,6 +378,8 @@ tool call
378378

379379
**Tool wall-clock budget vs. permission prompts.** Each tool `run()` is wrapped by an outer execution watchdog (`src/tui/tool-execution-watchdog.ts`). The watchdog arms only when Settings set `tools.timeoutMs` / `tools.maxTimeoutMs`, or when `run_shell` passes a positive timeout (requested plus slack, so this layer cannot beat shell-guard). The `task` tool is always exempt, regardless of Settings — a sub-agent run is bounded by its own limits (maxTurns, no-progress, thrash, opt-in deadlineMs), so the generic per-tool budget never aborts a healthy long-running worker; parent cancel, maxTurns, and eval `--agent-timeout-ms` still bound the run. By default (`tools.waitForApproval`, Settings → Tools, **On**), an armed budget freezes while the operator is deciding on a permission prompt, so a late approve still runs the tool and the agent waits for the decision instead of timing out under the modal. When **Off**, the budget keeps ticking during the prompt; if it expires first the tool is skipped and the permission modal is dismissed via the budget AbortSignal (auto-deny with a timeout message). The TUI permission queue (`src/tui/gate-wire.ts`, backed by `src/permission/queue.ts`) attaches that signal so ghost prompts cannot outlive an already-aborted tool.
380380

381+
`mcp__*` tool calls are the exception to "arms only when Settings set it": they arm unconditionally with a 5-minute default (`DEFAULT_MCP_TOOL_TIMEOUT_MS`), overridable via `mcp.timeoutMs` and still capped by `tools.maxTimeoutMs` (CL-6895). Nothing else bounds an MCP call — the stall watchdog treats an in-flight tool as activity by design, so a wedged MCP server previously hung a tool call, and the turn, forever. On expiry the call returns a normal tool-error result ("MCP tool `<name>` timed out after `<n>`s — the server may be wedged; retry or continue without it"); the turn is never aborted. The MCP client itself (`src/mcp/client.ts`, wrapping `@modelcontextprotocol/sdk`) multiplexes concurrent requests over one connection by JSON-RPC message id with no serial queue or mutex in our code or in the vendored SDK's `Protocol.request()` — so concurrent calls to the same server are not expected to deadlock each other. Live forensics for CL-6895 showed multi-minute MCP calls that eventually completed successfully, consistent with a slow server response rather than a client-side deadlock.
382+
381383
Approval scopes offered: Allow Once (persist nothing), Allow Always for a file or its directory (file tools), or a command shape (shell). There is intentionally no "all files" rung. Project-scoped Allow Always grants are confined to the session that minted them: they match the session root and its registered git worktrees (`cwdMatchesGrant` in `src/permission/authz-grants.ts` via `createWorktreeRootsProvider`), not bare process-cwd equality — so a grant at the repo root still covers a sub-agent running in a sibling worktree of the same project.
382384

383385
A queued gate's display-dependent timers (auto-deny timeout, tool-budget pause ceiling) arm when the request is actually shown to the operator, not when it is received — a request sitting behind others in the queue does not burn its timeout invisibly. `ask_operator` has the same abort/timeout safety net as the permission gate, so a queued operator question behind a stuck overlay cannot hang a run. Both live in `src/tui/gate-wire.ts`'s `onPermission`/`onOperator`.

docs/IMPLEMENTATION.md

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -233,6 +233,16 @@ Provider and model configuration lives in JSON settings files. The global file h
233233
- `timeoutMs` / `maxTimeoutMs` — outer execution watchdog around each tool `run()`. Unset leaves the watchdog unarmed; set these to arm it. `maxTimeoutMs` clamps non-shell tools when set and does not cap a longer requested `run_shell`. The `task` tool is always exempt: a dispatched sub-agent is bounded by its own limits (maxTurns, no-progress, thrash, opt-in `deadlineMs`), not the generic per-tool budget.
234234
- `waitForApproval` (default **true** when unset) — freeze that budget while a permission prompt is open so a late approve still runs the tool. **Settings → Tools** toggles this live for the next tool call and persists it here. When **false**, the budget keeps ticking during the prompt; on expiry the tool is skipped and the modal is auto-dismissed. The freeze is bounded: after **30 minutes** with the prompt still unanswered the budget resumes ticking on its own, so a prompt that never becomes visible (overlay open, UI gone) cannot hang a tool run indefinitely.
235235

236+
Optional `mcp` block bounds MCP tool calls (`mcp__*` names) specifically — unlike `tools.*`, this arms **unconditionally** even with no settings at all, defaulting to **5 minutes**, since a wedged MCP server otherwise hangs a call forever with nothing to bound it (CL-6895):
237+
238+
```json
239+
"mcp": {
240+
"timeoutMs": 300000
241+
}
242+
```
243+
244+
On expiry the call returns a normal tool-error result ("MCP tool `<name>` timed out after `<n>`s — the server may be wedged; retry or continue without it") that the model can react to; the turn itself is never aborted. `tools.maxTimeoutMs`, if set, still caps `mcp.timeoutMs`.
245+
236246
Optional `subagentMaxTurns` (integer **1–100**, default **30**) sets the default inference-turn budget for dispatched workers (not the parent chat session limit). Per-dispatch `task(maxTurns)` and agent profile `maxTurns` override this default; values above **100** are rejected on `task` and clamped for profiles. Always applies — the primary session is always orchestrator-capable (CL-5814).
237247

238248
Optional `sessionMode` is **deprecated**. Legacy values (`single` | `orchestrator`) may still appear on disk and load without error; resolve always returns **orchestrator**. There is no first-run mode picker and no Settings row. Both the interactive TUI (`runTUI`) and the non-TUI product path (`runExec` / `corbits exec`) are orchestrator-only. Exec bootstrap is otherwise a forked copy of the TUI path (shared stack, intentional deltas documented under Architecture → Exec Runner).
@@ -253,6 +263,7 @@ All `tools.*` keys live in the global settings file only — there is no per-rep
253263
| `tools.timeoutMs` | unset (watchdog unarmed) | Outer wall-clock budget per tool `run()` when set |
254264
| `tools.maxTimeoutMs` | unset | Cap on the outer budget when set; does not cap a longer requested `run_shell` |
255265
| `tools.waitForApproval` | `true` | Freeze the budget while a permission prompt is open (freeze capped at 30 min); `false` keeps the clock ticking and auto-dismisses the prompt on expiry |
266+
| `mcp.timeoutMs` | **300000** (5 min) — armed even when unset | Outer wall-clock budget for `mcp__*` tool calls specifically; capped by `tools.maxTimeoutMs` when set |
256267

257268
The `waitForApproval` default is resolved once at the watchdog boundary (`resolveWaitForApproval`); toggling **Settings → Tools** updates the live config for the next tool call and persists the value here.
258269

src/config/settings.ts

Lines changed: 22 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -135,6 +135,11 @@ export type Settings = {
135135
// the budget keeps ticking during the prompt; if it expires first the tool is
136136
// skipped and the prompt is dismissed.
137137
tools?: { timeoutMs?: number; maxTimeoutMs?: number; waitForApproval?: boolean };
138+
// Wall-clock budget for MCP tool calls specifically (mcp__* names). Unlike
139+
// the generic `tools` budget, this one is armed by default (see
140+
// DEFAULT_MCP_TOOL_TIMEOUT_MS) since a wedged MCP server otherwise hangs a
141+
// tool call forever with nothing to bound it.
142+
mcp?: { timeoutMs?: number };
138143
// Anonymous PostHog telemetry. Global only — never written to per-repo
139144
// local settings. `enabled` defaults to true (opt-out); `installationId`
140145
// is a random UUID generated once on first use; `noticeShown` stamps that
@@ -244,20 +249,23 @@ export function shellTimeoutFromSettings(
244249
};
245250
}
246251

247-
// Maps the settings tools block to the shape the tool-execution watchdog expects.
248-
// Returns undefined when nothing is configured so callers can skip the override.
252+
// Maps the settings tools/mcp blocks to the shape the tool-execution watchdog
253+
// expects. Returns undefined only when nothing at all is configured so callers
254+
// can skip the override; mcp.timeoutMs alone (with no tools.* set) still
255+
// produces a config, since MCP timeouts are armed unconditionally.
249256
export function toolWatchdogFromSettings(
250257
settings?: Settings | null,
251-
): { defaultMs?: number; maxMs?: number; waitForApproval?: boolean } | undefined {
258+
): { defaultMs?: number; maxMs?: number; waitForApproval?: boolean; mcpTimeoutMs?: number } | undefined {
252259
const tools = settings?.tools;
253-
if (tools === undefined) return undefined;
254-
const hasTimeout = tools.timeoutMs !== undefined || tools.maxTimeoutMs !== undefined;
255-
const hasWait = tools.waitForApproval !== undefined;
256-
if (!hasTimeout && !hasWait) return undefined;
260+
const mcpTimeoutMs = settings?.mcp?.timeoutMs;
261+
const hasTimeout = tools?.timeoutMs !== undefined || tools?.maxTimeoutMs !== undefined;
262+
const hasWait = tools?.waitForApproval !== undefined;
263+
if (!hasTimeout && !hasWait && mcpTimeoutMs === undefined) return undefined;
257264
return {
258-
...(tools.timeoutMs !== undefined ? { defaultMs: tools.timeoutMs } : {}),
259-
...(tools.maxTimeoutMs !== undefined ? { maxMs: tools.maxTimeoutMs } : {}),
260-
...(tools.waitForApproval !== undefined ? { waitForApproval: tools.waitForApproval } : {}),
265+
...(tools?.timeoutMs !== undefined ? { defaultMs: tools.timeoutMs } : {}),
266+
...(tools?.maxTimeoutMs !== undefined ? { maxMs: tools.maxTimeoutMs } : {}),
267+
...(tools?.waitForApproval !== undefined ? { waitForApproval: tools.waitForApproval } : {}),
268+
...(mcpTimeoutMs !== undefined ? { mcpTimeoutMs } : {}),
261269
};
262270
}
263271

@@ -464,6 +472,9 @@ const SettingsSchema = type({
464472
"maxTimeoutMs?": "number",
465473
"waitForApproval?": "boolean",
466474
}),
475+
"mcp?": type({
476+
"timeoutMs?": "number",
477+
}),
467478
"telemetry?": type({
468479
"enabled?": "boolean",
469480
"installationId?": "string",
@@ -771,6 +782,7 @@ export async function loadSettings(path: string): Promise<Settings | null> {
771782
: undefined,
772783
shell: s.shell as Settings["shell"] | undefined,
773784
tools: s.tools as Settings["tools"] | undefined,
785+
mcp: s.mcp as Settings["mcp"] | undefined,
774786
telemetry: s.telemetry as Settings["telemetry"] | undefined,
775787
otel: s.otel as Settings["otel"] | undefined,
776788
recentModels: s.recentModels as Settings["recentModels"] | undefined,

src/plugins/tool-time-budget.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,14 @@ export function formatToolExecutionTimeoutMessage(
3939
return `${trimmed}\n\n${notice}`;
4040
}
4141

42+
export function formatMcpToolTimeoutMessage(toolName: string, timeoutMs: number): string {
43+
const seconds = Math.round(timeoutMs / 1000);
44+
return (
45+
`MCP tool ${toolName} timed out after ${seconds}s — the server may be wedged; ` +
46+
`retry or continue without it.`
47+
);
48+
}
49+
4250
export function formatReadFileTimeoutMessage(
4351
path: string,
4452
partialResult?: string,

src/settings.test.ts

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -738,6 +738,22 @@ describe("loaders", () => {
738738
});
739739
expect(toolWatchdogFromSettings({ providers: {} })).toBeUndefined();
740740
});
741+
742+
test("toolWatchdogFromSettings maps mcp.timeoutMs alone (no tools.* set)", () => {
743+
expect(toolWatchdogFromSettings({ providers: {}, mcp: { timeoutMs: 45_000 } })).toEqual({
744+
mcpTimeoutMs: 45_000,
745+
});
746+
});
747+
748+
test("toolWatchdogFromSettings merges mcp.timeoutMs alongside tools.*", () => {
749+
expect(
750+
toolWatchdogFromSettings({
751+
providers: {},
752+
tools: { timeoutMs: 120_000, maxTimeoutMs: 600_000 },
753+
mcp: { timeoutMs: 45_000 },
754+
}),
755+
).toEqual({ defaultMs: 120_000, maxMs: 600_000, mcpTimeoutMs: 45_000 });
756+
});
741757
});
742758

743759
describe("persistSkipPermissionsDefault", () => {

src/tui/tool-execution-watchdog.test.ts

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { describe, expect, test } from "bun:test";
22
import type { AgentTool } from "@intx/agent";
33
import { createDynamicToolRunner } from "./dynamic-tool-runner.js";
44
import {
5+
DEFAULT_MCP_TOOL_TIMEOUT_MS,
56
MAX_TOOL_EXECUTION_TIMEOUT_MS,
67
RUN_SHELL_WATCHDOG_SLACK_MS,
78
getToolApprovalBudget,
@@ -122,6 +123,31 @@ describe("tool execution watchdog", () => {
122123
expect(resolveToolExecutionTimeoutMs({ defaultMs: 9_999_999, maxMs: 100 }, call)).toBe(100);
123124
});
124125

126+
test("mcp tool calls are bounded by default even with no config (CL-6895)", () => {
127+
const call = { id: "1", name: "mcp__linear__get_issue", arguments: {} };
128+
expect(resolveToolExecutionTimeoutMs(undefined, call)).toBe(DEFAULT_MCP_TOOL_TIMEOUT_MS);
129+
expect(resolveToolExecutionTimeoutMs({}, call)).toBe(DEFAULT_MCP_TOOL_TIMEOUT_MS);
130+
});
131+
132+
test("mcp.timeoutMs overrides the mcp default", () => {
133+
const call = { id: "1", name: "mcp__linear__get_issue", arguments: {} };
134+
expect(resolveToolExecutionTimeoutMs({ mcpTimeoutMs: 45_000 }, call)).toBe(45_000);
135+
});
136+
137+
test("tools.defaultMs alone (no mcpTimeoutMs) does not affect the mcp default", () => {
138+
const call = { id: "1", name: "mcp__linear__get_issue", arguments: {} };
139+
expect(resolveToolExecutionTimeoutMs({ defaultMs: 5_000 }, call)).toBe(
140+
DEFAULT_MCP_TOOL_TIMEOUT_MS,
141+
);
142+
});
143+
144+
test("tools.maxTimeoutMs still caps a longer mcp.timeoutMs override", () => {
145+
const call = { id: "1", name: "mcp__linear__get_issue", arguments: {} };
146+
expect(
147+
resolveToolExecutionTimeoutMs({ mcpTimeoutMs: 9_999_999, maxMs: 100 }, call),
148+
).toBe(100);
149+
});
150+
125151
test("withTimeout dispose clears timer without leaving hung state", async () => {
126152
const parent = new AbortController();
127153
const budget = withTimeout(parent.signal, 50);
@@ -158,6 +184,55 @@ describe("tool execution watchdog", () => {
158184
expect(result.content).toBe(formatToolExecutionTimeoutMessage("slow", 30));
159185
});
160186

187+
test(
188+
"mcp tool whose promise never resolves times out with a model-reactable error, turn continues",
189+
async () => {
190+
const runner = createDynamicToolRunner(
191+
[
192+
stringTool(
193+
"mcp__linear__get_issue",
194+
() => new Promise<string>(() => {}), // never resolves — wedged server
195+
),
196+
],
197+
{ mcpTimeoutMs: 30 },
198+
);
199+
const result = await runner.run(
200+
{ id: "1", name: "mcp__linear__get_issue", arguments: {} },
201+
new AbortController().signal,
202+
);
203+
expect(result.isError).toBe(true);
204+
expect(result.content).toContain("mcp__linear__get_issue timed out after 0s");
205+
expect(result.content).toContain("the server may be wedged");
206+
},
207+
10_000,
208+
);
209+
210+
test(
211+
"concurrent mcp tool calls each time out independently",
212+
async () => {
213+
const runner = createDynamicToolRunner(
214+
[
215+
stringTool("mcp__linear__get_issue", () => new Promise<string>(() => {})),
216+
stringTool("mcp__linear__list_issues", async () => "ok"),
217+
],
218+
{ mcpTimeoutMs: 30 },
219+
);
220+
const signal = new AbortController().signal;
221+
const [hung1, hung2, fast] = await Promise.all([
222+
runner.run({ id: "1", name: "mcp__linear__get_issue", arguments: {} }, signal),
223+
runner.run({ id: "2", name: "mcp__linear__get_issue", arguments: {} }, signal),
224+
runner.run({ id: "3", name: "mcp__linear__list_issues", arguments: {} }, signal),
225+
]);
226+
expect(hung1.isError).toBe(true);
227+
expect(hung1.content).toContain("mcp__linear__get_issue timed out");
228+
expect(hung2.isError).toBe(true);
229+
expect(hung2.content).toContain("mcp__linear__get_issue timed out");
230+
expect(fast.content).toBe("ok");
231+
expect(fast.isError).toBeUndefined();
232+
},
233+
10_000,
234+
);
235+
161236
test("parent cancel prefers execute salvage body over synthetic aborted", async () => {
162237
const parent = new AbortController();
163238
const salvage = {

src/tui/tool-execution-watchdog.ts

Lines changed: 41 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,9 @@
11
import { AsyncLocalStorage } from "node:async_hooks";
2-
import { formatToolExecutionTimeoutMessage } from "../plugins/tool-time-budget.js";
2+
import {
3+
formatMcpToolTimeoutMessage,
4+
formatToolExecutionTimeoutMessage,
5+
} from "../plugins/tool-time-budget.js";
6+
import { isMcpToolName } from "../mcp/tool-name.js";
37
import type { ToolCall, ToolResult } from "@intx/types/runtime";
48

59
/** Wall-clock budget for a single tool `run()` invocation (outer guard). */
@@ -13,8 +17,22 @@ export type ToolWatchdogConfig = {
1317
* is dismissed via the budget AbortSignal.
1418
*/
1519
waitForApproval?: boolean;
20+
/**
21+
* Override for mcp__* tool calls (settings.mcp.timeoutMs). Unlike the
22+
* generic defaultMs/maxMs pair, MCP tools are always bounded — a wedged MCP
23+
* server otherwise hangs a tool call forever (CL-6895) — so this only
24+
* changes the bound, it never leaves it unarmed.
25+
*/
26+
mcpTimeoutMs?: number;
1627
};
1728

29+
// Default wall-clock budget for a single MCP tool call when settings.mcp.timeoutMs
30+
// is unset. Live forensics (CL-6895) showed multi-minute MCP calls that were
31+
// merely slow and later completed successfully, not deadlocked — so this stays
32+
// generous (5 minutes) rather than the shorter default used for other tools,
33+
// while still bounding a genuinely wedged server.
34+
export const DEFAULT_MCP_TOOL_TIMEOUT_MS = 300_000;
35+
1836
// Cap applied when Settings set tools.timeoutMs without tools.maxTimeoutMs.
1937
// Not an implicit default — omitted settings leave the watchdog unarmed.
2038
export const MAX_TOOL_EXECUTION_TIMEOUT_MS = 1_800_000;
@@ -54,6 +72,11 @@ const BUDGET_EXPIRED = Symbol("tool-execution-budget-expired");
5472
* The task tool is exempt: it runs an entire sub-agent that carries its own
5573
* bounds (maxTurns, no-progress, thrash, opt-in deadline), so the generic
5674
* per-tool budget would abort healthy long-running workers mid-run.
75+
*
76+
* mcp__* tool calls are the opposite of exempt: they arm unconditionally (see
77+
* resolveMcpToolTimeoutMs) even when no Settings are configured, because an
78+
* MCP server can wedge a call forever with no other watchdog to bound it
79+
* (CL-6895).
5780
*/
5881
export function resolveToolExecutionTimeoutMs(
5982
config?: ToolWatchdogConfig,
@@ -66,9 +89,18 @@ export function resolveToolExecutionTimeoutMs(
6689
return requested + RUN_SHELL_WATCHDOG_SLACK_MS;
6790
}
6891
}
92+
if (call !== undefined && isMcpToolName(call.name)) {
93+
return resolveMcpToolTimeoutMs(config);
94+
}
6995
return resolveSettingsWatchdogTimeoutMs(config);
7096
}
7197

98+
function resolveMcpToolTimeoutMs(config: ToolWatchdogConfig | undefined): number {
99+
const max = config?.maxMs ?? MAX_TOOL_EXECUTION_TIMEOUT_MS;
100+
const raw = config?.mcpTimeoutMs ?? DEFAULT_MCP_TOOL_TIMEOUT_MS;
101+
return Math.min(max, Math.max(1, Math.floor(raw)));
102+
}
103+
72104
function requestedRunShellTimeoutMs(call: ToolCall): number | undefined {
73105
const timeout = call.arguments.timeout;
74106
if (typeof timeout !== "number" || !Number.isFinite(timeout) || timeout <= 0) {
@@ -283,6 +315,12 @@ function isAbortLikeToolError(content: string): boolean {
283315
return /abort/i.test(content);
284316
}
285317

318+
function formatTimeoutMessage(toolName: string, timeoutMs: number): string {
319+
return isMcpToolName(toolName)
320+
? formatMcpToolTimeoutMessage(toolName, timeoutMs)
321+
: formatToolExecutionTimeoutMessage(toolName, timeoutMs);
322+
}
323+
286324
/** True when execute produced a body the parent should prefer over synthetic abort/timeout. */
287325
export function isUsableToolExecuteResult(result: ToolResult): boolean {
288326
return (
@@ -402,7 +440,7 @@ export async function runWithToolExecutionWatchdog(
402440
void executePromise.catch(() => {});
403441
const content =
404442
timeoutMs !== undefined && !parentSignal.aborted
405-
? formatToolExecutionTimeoutMessage(call.name, timeoutMs)
443+
? formatTimeoutMessage(call.name, timeoutMs)
406444
: `${call.name} aborted`;
407445
return { callId: call.id, content, isError: true };
408446
}
@@ -425,7 +463,7 @@ export async function runWithToolExecutionWatchdog(
425463
) {
426464
return {
427465
callId: call.id,
428-
content: formatToolExecutionTimeoutMessage(call.name, timeoutMs),
466+
content: formatTimeoutMessage(call.name, timeoutMs),
429467
isError: true,
430468
};
431469
}

0 commit comments

Comments
 (0)