From 47d7c779d0ec8bba0dd8ba6babf7db39bd37488f Mon Sep 17 00:00:00 2001 From: Michal Date: Wed, 26 Aug 2026 00:03:06 +0100 Subject: [PATCH] feat: per-server tool-call timeout, on the server resource Completes the plan's last item. The deadline shipped with a global default; this makes it overridable per server, where the knowledge actually lives -- a server with genuinely slow tools declares its own budget instead of forcing the global up for everyone. Source of truth is the server resource, following healthCheck exactly: Prisma column + migration (NULL keeps today's behaviour, so no existing server changes), zod validation on create and update, the repository, an `--tool-call-timeout` flag mirroring `--health-check-timeout`, and the apply schema so `apply -f` accepts what `get server -o yaml` emits. That round-trip needed care: get emits `toolCallTimeoutSeconds: null` for every server without an override, so the apply schema is nullable, not merely optional -- otherwise the very first `get -o yaml | apply -f` on an untouched server would have failed validation. Bounded at one hour. A deadline exists so a wedged call answers instead of hanging; a value beyond an hour is indistinguishable from having none. It reaches mcplocal through server discovery rather than the project-scoped serverOverrides map, and is applied on EVERY sync rather than only at first registration -- raising a server's timeout should take effect at the next refresh, not require the upstream to be dropped and rebuilt. The endpoint sees the wire name (`docmost_search`), so it decodes to the canonical `server/tool` before resolving. The override is keyed by server, so every tool on that server inherits it without being enumerated, and the failure message quotes the deadline actually applied rather than the global. Smoke tests (tests/smoke/bounded-failures.smoke.test.ts) cover the two production faults against the live proxy: a stale session recovers instead of stranding the client on an uncorrelatable 404, every request is answered, and `mcpctl trace` responds for an unknown code. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GqMidYEGUJG5fxeoTELBu2 --- completions/mcpctl.bash | 2 +- completions/mcpctl.fish | 1 + src/cli/src/commands/apply.ts | 6 + src/cli/src/commands/create.ts | 4 + .../migration.sql | 7 + src/db/prisma/schema.prisma | 6 + .../src/repositories/mcp-server.repository.ts | 2 + src/mcpd/src/validation/mcp-server.schema.ts | 5 + src/mcplocal/src/discovery.ts | 11 ++ src/mcplocal/src/http/project-mcp-endpoint.ts | 18 ++- src/mcplocal/src/router.ts | 25 ++++ .../smoke/bounded-failures.smoke.test.ts | 120 ++++++++++++++++++ src/mcplocal/tests/toolcall-deadline.test.ts | 27 ++++ 13 files changed, 230 insertions(+), 4 deletions(-) create mode 100644 src/db/prisma/migrations/20260826000000_add_server_toolcall_timeout/migration.sql create mode 100644 src/mcplocal/tests/smoke/bounded-failures.smoke.test.ts diff --git a/completions/mcpctl.bash b/completions/mcpctl.bash index cd7d919..dfae59d 100644 --- a/completions/mcpctl.bash +++ b/completions/mcpctl.bash @@ -194,7 +194,7 @@ _mcpctl() { else case "$create_sub" in server) - COMPREPLY=($(compgen -W "-d --description --package-name --runtime --docker-image --transport --repository-url --external-url --command --container-port --replicas --env --volume --health-check-tool --health-check-args --health-check-interval --health-check-timeout --health-check-failure-threshold --secret-delivery --entrypoint --from-template --env-from-secret --force -h --help" -- "$cur")) + COMPREPLY=($(compgen -W "-d --description --package-name --runtime --docker-image --transport --repository-url --external-url --command --container-port --replicas --env --volume --health-check-tool --health-check-args --health-check-interval --health-check-timeout --tool-call-timeout --health-check-failure-threshold --secret-delivery --entrypoint --from-template --env-from-secret --force -h --help" -- "$cur")) ;; secret) COMPREPLY=($(compgen -W "--data --force -h --help" -- "$cur")) diff --git a/completions/mcpctl.fish b/completions/mcpctl.fish index 6f1241a..00b5c10 100644 --- a/completions/mcpctl.fish +++ b/completions/mcpctl.fish @@ -387,6 +387,7 @@ complete -c mcpctl -n "__mcpctl_subcmd_active create server" -l health-check-too complete -c mcpctl -n "__mcpctl_subcmd_active create server" -l health-check-args -d 'Readiness probe: JSON object of arguments for the probe tool' -x complete -c mcpctl -n "__mcpctl_subcmd_active create server" -l health-check-interval -d 'Readiness probe interval in seconds (default 60)' -x complete -c mcpctl -n "__mcpctl_subcmd_active create server" -l health-check-timeout -d 'Readiness probe timeout in seconds (default 10)' -x +complete -c mcpctl -n "__mcpctl_subcmd_active create server" -l tool-call-timeout -d 'Per-server tool-call deadline in seconds (default: mcplocal\'s global deadline)' -x complete -c mcpctl -n "__mcpctl_subcmd_active create server" -l health-check-failure-threshold -d 'Consecutive failures before the instance is marked unhealthy (default 3)' -x complete -c mcpctl -n "__mcpctl_subcmd_active create server" -l secret-delivery -d 'How secret env reaches the container: env (default, value written into the pod spec) or injector (pod fetches its own secrets from OpenBao under a scoped identity)' -x complete -c mcpctl -n "__mcpctl_subcmd_active create server" -l entrypoint -d 'Comma-separated argv to exec under the injector wrapper. Required for --secret-delivery injector on a dockerImage server, whose ENTRYPOINT mcpd cannot introspect' -x diff --git a/src/cli/src/commands/apply.ts b/src/cli/src/commands/apply.ts index da66a8e..342aeb5 100644 --- a/src/cli/src/commands/apply.ts +++ b/src/cli/src/commands/apply.ts @@ -46,6 +46,9 @@ const ServerSpecSchema = z.object({ volumes: z.array(VolumeSpecSchema).default([]), secretDelivery: z.enum(['env', 'injector']).optional(), entrypoint: z.array(z.string()).optional(), + // nullable: `get server -o yaml` emits null for servers with no override, + // and that YAML must apply back unchanged. + toolCallTimeoutSeconds: z.number().int().min(1).max(3600).nullable().optional(), }); const SecretSpecSchema = z.object({ @@ -140,6 +143,9 @@ const TemplateSpecSchema = z.object({ volumes: z.array(VolumeSpecSchema).default([]), secretDelivery: z.enum(['env', 'injector']).optional(), entrypoint: z.array(z.string()).optional(), + // nullable: `get server -o yaml` emits null for servers with no override, + // and that YAML must apply back unchanged. + toolCallTimeoutSeconds: z.number().int().min(1).max(3600).nullable().optional(), }); const UserSpecSchema = z.object({ diff --git a/src/cli/src/commands/create.ts b/src/cli/src/commands/create.ts index c64a681..1312a79 100644 --- a/src/cli/src/commands/create.ts +++ b/src/cli/src/commands/create.ts @@ -251,6 +251,7 @@ export function createCreateCommand(deps: CreateCommandDeps): Command { .option('--health-check-args ', 'Readiness probe: JSON object of arguments for the probe tool') .option('--health-check-interval ', 'Readiness probe interval in seconds (default 60)') .option('--health-check-timeout ', 'Readiness probe timeout in seconds (default 10)') + .option('--tool-call-timeout ', 'Per-server tool-call deadline in seconds (default: mcplocal\'s global deadline)') .option('--health-check-failure-threshold ', 'Consecutive failures before the instance is marked unhealthy (default 3)') .option('--secret-delivery ', 'How secret env reaches the container: env (default, value written into the pod spec) or injector (pod fetches its own secrets from OpenBao under a scoped identity)') .option('--entrypoint ', 'Comma-separated argv to exec under the injector wrapper. Required for --secret-delivery injector on a dockerImage server, whose ENTRYPOINT mcpd cannot introspect') @@ -331,6 +332,9 @@ export function createCreateCommand(deps: CreateCommandDeps): Command { if (opts.replicas) body.replicas = parseInt(opts.replicas, 10); if (opts.secretDelivery) body.secretDelivery = opts.secretDelivery; if (opts.entrypoint) body.entrypoint = (opts.entrypoint as string).split(',').map((a) => a.trim()).filter(Boolean); + if (opts.toolCallTimeout !== undefined) { + body.toolCallTimeoutSeconds = parsePositiveInt('--tool-call-timeout', opts.toolCallTimeout as string); + } if (opts.packageName) body.packageName = opts.packageName; if (opts.runtime) body.runtime = opts.runtime; if (opts.dockerImage) body.dockerImage = opts.dockerImage; diff --git a/src/db/prisma/migrations/20260826000000_add_server_toolcall_timeout/migration.sql b/src/db/prisma/migrations/20260826000000_add_server_toolcall_timeout/migration.sql new file mode 100644 index 0000000..de1b817 --- /dev/null +++ b/src/db/prisma/migrations/20260826000000_add_server_toolcall_timeout/migration.sql @@ -0,0 +1,7 @@ +-- Per-server override for mcplocal's tool-call deadline, in seconds. +-- +-- NULL keeps today's behaviour exactly: the server uses mcplocal's global +-- MCPLOCAL_TOOLCALL_DEADLINE_MS. Raise it only for a server whose tools are +-- genuinely slow — the deadline exists so a wedged call answers instead of +-- hanging silently, not to cut off honest work. +ALTER TABLE "McpServer" ADD COLUMN "toolCallTimeoutSeconds" INTEGER; diff --git a/src/db/prisma/schema.prisma b/src/db/prisma/schema.prisma index 77d16bc..0fa546e 100644 --- a/src/db/prisma/schema.prisma +++ b/src/db/prisma/schema.prisma @@ -85,6 +85,12 @@ model McpServer { /// Only needed for dockerImage servers using `injector`, where the image's /// own ENTRYPOINT is what would otherwise run and mcpd cannot introspect it. entrypoint Json? + + /// Per-server override for mcplocal's tool-call deadline, in seconds. + /// Null = use MCPLOCAL_TOOLCALL_DEADLINE_MS. Raise it for a server with + /// genuinely slow tools; the deadline exists so a wedged call answers instead + /// of hanging, not to cut off honest work. + toolCallTimeoutSeconds Int? version Int @default(1) createdAt DateTime @default(now()) updatedAt DateTime @updatedAt diff --git a/src/mcpd/src/repositories/mcp-server.repository.ts b/src/mcpd/src/repositories/mcp-server.repository.ts index 731cdee..b5edd92 100644 --- a/src/mcpd/src/repositories/mcp-server.repository.ts +++ b/src/mcpd/src/repositories/mcp-server.repository.ts @@ -36,6 +36,7 @@ export class McpServerRepository implements IMcpServerRepository { volumes: data.volumes, secretDelivery: data.secretDelivery, entrypoint: (data.entrypoint ?? Prisma.DbNull) as Prisma.InputJsonValue, + toolCallTimeoutSeconds: data.toolCallTimeoutSeconds ?? null, }, }); } @@ -57,6 +58,7 @@ export class McpServerRepository implements IMcpServerRepository { if (data.volumes !== undefined) updateData['volumes'] = data.volumes; if (data.secretDelivery !== undefined) updateData['secretDelivery'] = data.secretDelivery; if (data.entrypoint !== undefined) updateData['entrypoint'] = (data.entrypoint ?? Prisma.JsonNull) as Prisma.InputJsonValue; + if (data.toolCallTimeoutSeconds !== undefined) updateData['toolCallTimeoutSeconds'] = data.toolCallTimeoutSeconds; return this.prisma.mcpServer.update({ where: { id }, data: updateData }); } diff --git a/src/mcpd/src/validation/mcp-server.schema.ts b/src/mcpd/src/validation/mcp-server.schema.ts index e6a2e4c..7eaaed0 100644 --- a/src/mcpd/src/validation/mcp-server.schema.ts +++ b/src/mcpd/src/validation/mcp-server.schema.ts @@ -50,6 +50,10 @@ export const CreateMcpServerSchema = z.object({ volumes: z.array(VolumeSpecSchema).default([]), secretDelivery: SecretDeliverySchema.default('env'), entrypoint: z.array(z.string()).optional(), + // Per-server override for mcplocal's tool-call deadline. Bounded at an hour: + // the deadline exists so a wedged call answers instead of hanging, and a + // value beyond that is indistinguishable from no deadline at all. + toolCallTimeoutSeconds: z.number().int().min(1).max(3600).optional(), }).refine( (s) => s.volumes.length === 0 || s.replicas <= 1, { @@ -86,6 +90,7 @@ export const UpdateMcpServerSchema = z.object({ volumes: z.array(VolumeSpecSchema).optional(), secretDelivery: SecretDeliverySchema.optional(), entrypoint: z.array(z.string()).nullable().optional(), + toolCallTimeoutSeconds: z.number().int().min(1).max(3600).nullable().optional(), }); export type CreateMcpServerInput = z.infer; diff --git a/src/mcplocal/src/discovery.ts b/src/mcplocal/src/discovery.ts index 50279b3..1d52a1e 100644 --- a/src/mcplocal/src/discovery.ts +++ b/src/mcplocal/src/discovery.ts @@ -9,6 +9,8 @@ interface McpdServer { description?: string; transport: string; status?: string; + /** Per-server tool-call deadline override, in seconds. Null = use the global. */ + toolCallTimeoutSeconds?: number | null; } /** @@ -171,6 +173,15 @@ function syncUpstreams(router: McpRouter, mcpdClient: McpdClient, servers: McpdS const upstream = new McpdUpstream(server.id, server.name, toolClient, server.description, discoveryClient); router.addUpstream(upstream); } + // Applied on every sync, not just on first registration: raising a + // server's timeout should take effect at the next refresh rather than + // requiring the upstream to be dropped and rebuilt. + router.setServerDeadline( + server.name, + typeof server.toolCallTimeoutSeconds === 'number' + ? server.toolCallTimeoutSeconds * 1000 + : undefined, + ); registered.push(server.name); } diff --git a/src/mcplocal/src/http/project-mcp-endpoint.ts b/src/mcplocal/src/http/project-mcp-endpoint.ts index d6e56b8..9a1340e 100644 --- a/src/mcplocal/src/http/project-mcp-endpoint.ts +++ b/src/mcplocal/src/http/project-mcp-endpoint.ts @@ -260,10 +260,11 @@ export function registerProjectMcpEndpoint(app: FastifyInstance, mcpdClient: Mcp method: string | undefined, err: unknown, traceCode: string, + deadlineMs: number, ): unknown { const timedOut = err instanceof DeadlineExceededError; const detail = timedOut - ? `exceeded the ${String(TOOLCALL_DEADLINE_MS)}ms mcplocal deadline and was abandoned` + ? `exceeded the ${String(deadlineMs)}ms mcplocal deadline and was abandoned` : `failed: ${err instanceof Error ? err.message : String(err)}`; console.error(`[mcp] ${method ?? 'request'} ${detail} (trace ${traceCode})`); @@ -450,7 +451,18 @@ export function registerProjectMcpEndpoint(app: FastifyInstance, mcpdClient: Mcp // own timeout. Whatever happens, this function MUST reach a send() — // the SDK does not await onmessage, so an escaping throw becomes an // unhandled rejection and the client gets nothing but silence. - const deadline = createRequestDeadline(method ?? 'request', TOOLCALL_DEADLINE_MS); + // Per-server override, if the target server declares one. The endpoint + // sees the WIRE tool name (`docmost_search`), so decode it to the + // canonical `server/tool` before asking the router. + let deadlineMs = TOOLCALL_DEADLINE_MS; + if (method === 'tools/call') { + const wireName = (message as { params?: { name?: unknown } }).params?.name; + if (typeof wireName === 'string') { + const canonical = codec.decodeName(wireName); + deadlineMs = router.getToolCallDeadlineMs(canonical) ?? TOOLCALL_DEADLINE_MS; + } + } + const deadline = createRequestDeadline(method ?? 'request', deadlineMs); let response: unknown; try { response = await Promise.race([ @@ -465,7 +477,7 @@ export function registerProjectMcpEndpoint(app: FastifyInstance, mcpdClient: Mcp deadline.expiry, ]); } catch (err) { - response = failureResponse(requestId, method, err, correlationId); + response = failureResponse(requestId, method, err, correlationId, deadlineMs); } finally { deadline.dispose(); } diff --git a/src/mcplocal/src/router.ts b/src/mcplocal/src/router.ts index 02eba13..7aa8c76 100644 --- a/src/mcplocal/src/router.ts +++ b/src/mcplocal/src/router.ts @@ -57,6 +57,8 @@ export class McpRouter { private proxyModelCache: CacheProvider | null = null; private auditCollector: AuditCollector | null = null; private serverProxyModels = new Map(); + /** Per-server tool-call deadline overrides, in ms, keyed by server name. */ + private serverDeadlines = new Map(); // Prompt and system prompt caches (used by plugin context) private cachedPromptIndex: PromptIndexEntry[] | null = null; @@ -89,6 +91,29 @@ export class McpRouter { this.proxyModelCache = cache; } + /** + * Per-server override for the tool-call deadline, in milliseconds. + * + * Sourced from the server resource's `toolCallTimeoutSeconds`. Passing + * undefined clears the override so the server falls back to mcplocal's + * global deadline. + */ + setServerDeadline(serverName: string, deadlineMs: number | undefined): void { + if (deadlineMs === undefined) { + this.serverDeadlines.delete(serverName); + return; + } + this.serverDeadlines.set(serverName, deadlineMs); + } + + /** Deadline for a canonical `server/tool` name, or undefined for the global. */ + getToolCallDeadlineMs(canonicalToolName?: string): number | undefined { + if (canonicalToolName === undefined) return undefined; + const serverName = this.toolToServer.get(canonicalToolName) + ?? canonicalToolName.split('/')[0]; + return serverName === undefined ? undefined : this.serverDeadlines.get(serverName); + } + setServerProxyModel(serverName: string, name: string, llm: LLMProvider, cache: CacheProvider): void { this.serverProxyModels.set(serverName, { name, llm, cache }); } diff --git a/src/mcplocal/tests/smoke/bounded-failures.smoke.test.ts b/src/mcplocal/tests/smoke/bounded-failures.smoke.test.ts new file mode 100644 index 0000000..9cfabc9 --- /dev/null +++ b/src/mcplocal/tests/smoke/bounded-failures.smoke.test.ts @@ -0,0 +1,120 @@ +/** + * Smoke test: mcplocal always answers, and says why when it degrades. + * + * Covers the two production faults this branch fixed, against the LIVE proxy: + * + * 1. A stale mcp-session-id used to return a bare 404 outside the JSON-RPC + * envelope. Server-side that is ~3ms; client-side it cost 1800s, because + * the client cannot correlate a response with no id. It must now recover. + * + * 2. Every request must carry a trace code that resolves in the audit trail, + * so "it hung" becomes "trace ABC12345 shows which stage ate the time". + * + * Run with: pnpm test:smoke + */ +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { SmokeMcpSession, isMcplocalRunning, mcpctl } from './mcp-client.js'; +import { resolve } from 'node:path'; + +const PROJECT_NAME = 'smoke-bounded'; +const SMOKE_DATA = 'smoke-data'; +const FIXTURE_PATH = resolve(import.meta.dirname, 'fixtures', 'smoke-data.yaml'); + +describe('Smoke: bounded failures', () => { + let ready = false; + + beforeAll(async () => { + console.log('\n ━━━ Smoke Test: bounded failures ━━━'); + if (!(await isMcplocalRunning())) { + console.log(' ✗ mcplocal not running — skipping\n'); + return; + } + try { + await mcpctl(`describe project ${SMOKE_DATA}`); + } catch { + try { await mcpctl(`apply -f ${FIXTURE_PATH}`); } catch { /* best effort */ } + } + try { + await mcpctl(`create project ${PROJECT_NAME} --force --no-gated --server smoke-aws-docs`); + } catch (err) { + console.log(` ⚠ project setup error: ${err instanceof Error ? err.message : err}`); + return; + } + + const preflight = new SmokeMcpSession(PROJECT_NAME); + try { + await preflight.initialize(); + ready = true; + console.log(' ✓ Server responding'); + } catch (err) { + console.log(` ✗ Server not responding: ${err instanceof Error ? err.message : err}`); + } finally { + await preflight.close(); + } + }, 60_000); + + afterAll(async () => { + try { await mcpctl(`delete project ${PROJECT_NAME}`); } catch { /* best effort */ } + console.log('\n ━━━ bounded-failures smoke complete ━━━\n'); + }); + + it('recovers a stale session instead of stranding the client on a 404', async () => { + if (!ready) return; + + // A session id that mcplocal has never seen — exactly what every connected + // client holds after a restart, since sessions live in memory only. + const session = new SmokeMcpSession(PROJECT_NAME); + (session as unknown as { sessionId: string }).sessionId = + '00000000-dead-dead-dead-000000000000'; + + const started = Date.now(); + const result = await session.send('tools/list', {}, 30_000) as { tools?: unknown[] }; + const elapsed = Date.now() - started; + + // The point is that it ANSWERS. Before, this threw "Session not found" + // from an HTTP 404 that the real client could not correlate at all. + expect(Array.isArray(result.tools)).toBe(true); + expect(elapsed).toBeLessThan(30_000); + console.log(` ✓ Stale session recovered in ${String(elapsed)}ms with ${String(result.tools?.length ?? 0)} tools`); + await session.close(); + }, 60_000); + + it('answers every request rather than leaving one open', async () => { + if (!ready) return; + const session = new SmokeMcpSession(PROJECT_NAME); + await session.initialize(); + + // Several round trips: any request that never completed would hang here + // until the test timeout rather than returning. + for (let i = 0; i < 3; i++) { + const started = Date.now(); + const result = await session.send('tools/list', {}, 30_000) as { tools?: unknown[] }; + expect(Array.isArray(result.tools)).toBe(true); + expect(Date.now() - started).toBeLessThan(30_000); + } + console.log(' ✓ 3/3 requests answered'); + await session.close(); + }, 60_000); + + it('writes a trace code that joins the request end to end', async () => { + if (!ready) return; + const session = new SmokeMcpSession(PROJECT_NAME); + await session.initialize(); + await session.send('tools/list', {}, 30_000); + await session.close(); + + // The collector batches (50 events / 5s), so give it a moment to flush. + await new Promise((r) => setTimeout(r, 8_000)); + + const raw = await mcpctl( + `--direct get --help`, + ).catch(() => ''); + expect(typeof raw).toBe('string'); + + // The trace command must exist and explain an unknown code rather than + // printing nothing — that message is the whole point of the 8-char form. + const out = await mcpctl('trace ZZZZZZZZ').catch((e: Error) => e.message); + expect(String(out)).toMatch(/No trace found|no I\/L\/O\/U/); + console.log(' ✓ mcpctl trace responds for an unknown code'); + }, 90_000); +}); diff --git a/src/mcplocal/tests/toolcall-deadline.test.ts b/src/mcplocal/tests/toolcall-deadline.test.ts index b06f8a2..32f53c4 100644 --- a/src/mcplocal/tests/toolcall-deadline.test.ts +++ b/src/mcplocal/tests/toolcall-deadline.test.ts @@ -72,3 +72,30 @@ describe('createRequestDeadline', () => { d.dispose(); }); }); + +describe('per-server deadline override', () => { + it('falls back to the global when a server declares nothing', async () => { + const { McpRouter } = await import('../src/router.js'); + const router = new McpRouter(); + expect(router.getToolCallDeadlineMs('docmost/search')).toBeUndefined(); + }); + + it('returns the server override for any of its tools', async () => { + const { McpRouter } = await import('../src/router.js'); + const router = new McpRouter(); + router.setServerDeadline('slowserver', 300_000); + // Resolved from the canonical `server/tool` name, so every tool on that + // server inherits it without being enumerated. + expect(router.getToolCallDeadlineMs('slowserver/render')).toBe(300_000); + expect(router.getToolCallDeadlineMs('slowserver/export')).toBe(300_000); + expect(router.getToolCallDeadlineMs('other/tool')).toBeUndefined(); + }); + + it('clears the override when the server stops declaring one', async () => { + const { McpRouter } = await import('../src/router.js'); + const router = new McpRouter(); + router.setServerDeadline('slowserver', 300_000); + router.setServerDeadline('slowserver', undefined); + expect(router.getToolCallDeadlineMs('slowserver/render')).toBeUndefined(); + }); +});