diff --git a/completions/mcpctl.bash b/completions/mcpctl.bash index dfae59d..4a3a277 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 --tool-call-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 --memory-limit-mb --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 00b5c10..1fc5100 100644 --- a/completions/mcpctl.fish +++ b/completions/mcpctl.fish @@ -388,6 +388,7 @@ complete -c mcpctl -n "__mcpctl_subcmd_active create server" -l health-check-arg 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 memory-limit-mb -d 'Per-server memory ceiling in MiB (default: 512). Raise it for a server that drives a browser or holds a large index — at the default it is OOMKilled with no error, just a restart' -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/docs/reliability.md b/docs/reliability.md index 0eae89f..2611cf9 100644 --- a/docs/reliability.md +++ b/docs/reliability.md @@ -137,6 +137,42 @@ Also check the *dialect*: UniFi's `controller_type` must be `classic` for a self-hosted controller (login `/api/login`, no `/proxy/network` prefix). `unifi_os` sends every request to a path that 404s. +### A server that restarts instead of erroring is out of memory + +Server pods get **512 MiB** by default (`DEFAULT_MEMORY_LIMIT`). That is ample +for a server that proxies an API and nowhere near enough for one that drives a +browser or holds an index in memory. + +An OOMKill is the quietest failure in the fleet, because **nothing reports an +error**. The kernel kills the container, Kubernetes restarts it, the readiness +probe passes again, and `mcpctl get instances` reads `healthy`. Whatever the +server was doing is simply gone — for `docs`, six scrape jobs whose queue lived +in memory, leaving an index with 17 pages in it and no failed job to look at. + +The tells, in order of how fast they answer the question: + +```bash +kubectl -n mcpctl-servers get pods | grep # RESTARTS climbing +kubectl -n mcpctl-servers get pod -o jsonpath='{.status.containerStatuses[0].lastState.terminated.reason}' +``` + +`OOMKilled` there is conclusive. `mcpctl logs` will not show it: the process +never got to say anything. + +Raise the ceiling per server rather than for the fleet — most servers do not +need it, and a bigger default wastes real memory on every node: + +```bash +mcpctl create server docs --memory-limit-mb 2048 --force +``` + +Declared in MiB, stored on the server, converted to bytes in the container +spec. Null keeps the 512 MiB default, so existing servers are unchanged. Sizing +rule of thumb: measure idle first (`/sys/fs/cgroup/memory.current` inside the +pod), then leave headroom for the peak — `docs` idles at ~228 MiB and crosses +512 MiB within seconds of a scrape, because each page render is a Chromium +process. + ## LLM-*essential* operations — failover chain Chat needs *an* LLM but not a *specific* one. Instead of failing when the pinned diff --git a/src/cli/src/commands/apply.ts b/src/cli/src/commands/apply.ts index 342aeb5..04d322c 100644 --- a/src/cli/src/commands/apply.ts +++ b/src/cli/src/commands/apply.ts @@ -49,6 +49,7 @@ const ServerSpecSchema = z.object({ // 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(), + memoryLimitMb: z.number().int().min(64).max(16384).nullable().optional(), }); const SecretSpecSchema = z.object({ diff --git a/src/cli/src/commands/create.ts b/src/cli/src/commands/create.ts index 1312a79..2fcecc8 100644 --- a/src/cli/src/commands/create.ts +++ b/src/cli/src/commands/create.ts @@ -252,6 +252,7 @@ export function createCreateCommand(deps: CreateCommandDeps): Command { .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('--memory-limit-mb ', 'Per-server memory ceiling in MiB (default: 512). Raise it for a server that drives a browser or holds a large index — at the default it is OOMKilled with no error, just a restart') .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') @@ -335,6 +336,9 @@ export function createCreateCommand(deps: CreateCommandDeps): Command { if (opts.toolCallTimeout !== undefined) { body.toolCallTimeoutSeconds = parsePositiveInt('--tool-call-timeout', opts.toolCallTimeout as string); } + if (opts.memoryLimitMb !== undefined) { + body.memoryLimitMb = parsePositiveInt('--memory-limit-mb', opts.memoryLimitMb 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/20260917000000_add_server_memory_limit/migration.sql b/src/db/prisma/migrations/20260917000000_add_server_memory_limit/migration.sql new file mode 100644 index 0000000..ad07a51 --- /dev/null +++ b/src/db/prisma/migrations/20260917000000_add_server_memory_limit/migration.sql @@ -0,0 +1,9 @@ +-- Per-server memory ceiling, in MiB. +-- +-- NULL keeps today's behaviour exactly: the server gets the orchestrator's +-- DEFAULT_MEMORY_LIMIT (512 MiB). That default is fine for a server that +-- proxies an API, and fatal for one that drives a browser — docs-mcp-server +-- scrapes with headless Chromium, sits at ~228 MiB idle, and crosses 512 MiB +-- within seconds of the first scrape. The pod is OOMKilled and restarts, so +-- the only trace is a restart count and an empty job queue. +ALTER TABLE "McpServer" ADD COLUMN "memoryLimitMb" INTEGER; diff --git a/src/db/prisma/schema.prisma b/src/db/prisma/schema.prisma index 0fa546e..a38497f 100644 --- a/src/db/prisma/schema.prisma +++ b/src/db/prisma/schema.prisma @@ -91,6 +91,13 @@ model McpServer { /// genuinely slow tools; the deadline exists so a wedged call answers instead /// of hanging, not to cut off honest work. toolCallTimeoutSeconds Int? + + /// Per-server memory ceiling in MiB. Null = the orchestrator's 512 MiB + /// default. Raise it for a server that legitimately needs more than that: + /// docs-mcp-server scrapes with headless Chromium and is OOMKilled at the + /// default before it indexes a single page, with nothing in the logs to say + /// so — the pod simply restarts and the job queue is gone. + memoryLimitMb 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 b5edd92..bb48743 100644 --- a/src/mcpd/src/repositories/mcp-server.repository.ts +++ b/src/mcpd/src/repositories/mcp-server.repository.ts @@ -37,6 +37,7 @@ export class McpServerRepository implements IMcpServerRepository { secretDelivery: data.secretDelivery, entrypoint: (data.entrypoint ?? Prisma.DbNull) as Prisma.InputJsonValue, toolCallTimeoutSeconds: data.toolCallTimeoutSeconds ?? null, + memoryLimitMb: data.memoryLimitMb ?? null, }, }); } @@ -59,6 +60,7 @@ export class McpServerRepository implements IMcpServerRepository { 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; + if (data.memoryLimitMb !== undefined) updateData['memoryLimitMb'] = data.memoryLimitMb; return this.prisma.mcpServer.update({ where: { id }, data: updateData }); } diff --git a/src/mcpd/src/services/instance.service.ts b/src/mcpd/src/services/instance.service.ts index 4e68821..d81e657 100644 --- a/src/mcpd/src/services/instance.service.ts +++ b/src/mcpd/src/services/instance.service.ts @@ -481,6 +481,16 @@ export class InstanceService { spec.containerPort = server.containerPort ?? 3000; } + // Per-server memory ceiling. Null leaves DEFAULT_MEMORY_LIMIT (512 MiB) + // in place, which is right for an API proxy and fatal for a server that + // drives a browser: docs-mcp-server was OOMKilled seconds into every + // scrape, and an OOMKill leaves no error — the pod restarts, the + // in-memory job queue is gone, and the instance reports healthy again. + const memoryLimitMb = server.memoryLimitMb as number | null; + if (memoryLimitMb !== null && memoryLimitMb !== undefined) { + spec.memoryLimit = memoryLimitMb * 1024 * 1024; + } + // Volumes are keyed on the server, not this instance: `spec.name` carries // the instance id and a claim named after it would be torn down on the // next server edit — exactly when the data has to survive. diff --git a/src/mcpd/src/validation/mcp-server.schema.ts b/src/mcpd/src/validation/mcp-server.schema.ts index 7eaaed0..6ef0e36 100644 --- a/src/mcpd/src/validation/mcp-server.schema.ts +++ b/src/mcpd/src/validation/mcp-server.schema.ts @@ -54,6 +54,10 @@ export const CreateMcpServerSchema = z.object({ // 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(), + // Per-server memory ceiling in MiB. Floor 64 because nothing useful runs + // below it; ceiling 16384 so a typo cannot ask for a pod no node can + // schedule, which fails as a silently Pending instance rather than an error. + memoryLimitMb: z.number().int().min(64).max(16384).optional(), }).refine( (s) => s.volumes.length === 0 || s.replicas <= 1, { @@ -91,6 +95,7 @@ export const UpdateMcpServerSchema = z.object({ secretDelivery: SecretDeliverySchema.optional(), entrypoint: z.array(z.string()).nullable().optional(), toolCallTimeoutSeconds: z.number().int().min(1).max(3600).nullable().optional(), + memoryLimitMb: z.number().int().min(64).max(16384).nullable().optional(), }); export type CreateMcpServerInput = z.infer; diff --git a/src/mcpd/tests/instance-service.test.ts b/src/mcpd/tests/instance-service.test.ts index 2e4d89e..5d860e0 100644 --- a/src/mcpd/tests/instance-service.test.ts +++ b/src/mcpd/tests/instance-service.test.ts @@ -70,7 +70,7 @@ function mockOrchestrator(): McpOrchestrator { }; } -function makeServer(overrides: Partial<{ id: string; name: string; replicas: number; dockerImage: string | null; externalUrl: string | null; transport: string; command: unknown; containerPort: number | null; volumes: unknown }> = {}) { +function makeServer(overrides: Partial<{ id: string; name: string; replicas: number; dockerImage: string | null; externalUrl: string | null; transport: string; command: unknown; containerPort: number | null; volumes: unknown; memoryLimitMb: number | null }> = {}) { return { id: overrides.id ?? 'srv-1', name: overrides.name ?? 'slack', @@ -83,6 +83,7 @@ function makeServer(overrides: Partial<{ id: string; name: string; replicas: num externalUrl: overrides.externalUrl ?? null, command: overrides.command ?? null, containerPort: overrides.containerPort ?? null, + memoryLimitMb: overrides.memoryLimitMb ?? null, replicas: overrides.replicas ?? 1, env: [], version: 1, @@ -191,6 +192,37 @@ describe('InstanceService', () => { }); }); + describe('memory limit', () => { + // A server that drives a browser (docs-mcp-server scrapes with headless + // Chromium) is OOMKilled seconds into its first job at the orchestrator's + // 512 MiB default, and an OOMKill says nothing: the pod restarts, the + // in-memory job queue is gone, and the instance reads healthy again. + it('converts the declared MiB ceiling into the spec, in bytes', async () => { + vi.mocked(serverRepo.findById).mockResolvedValue(makeServer({ + name: 'docs', + memoryLimitMb: 2048, + })); + vi.mocked(instanceRepo.findAll).mockResolvedValue([]); + + await service.reconcile('srv-1'); + + const spec = vi.mocked(orchestrator.createContainer).mock.calls[0]![0]; + expect(spec.memoryLimit).toBe(2048 * 1024 * 1024); + }); + + it('leaves the spec free of a limit when the server declares none', async () => { + // Absent, not zero: the orchestrator falls back to DEFAULT_MEMORY_LIMIT, + // so every existing server keeps exactly the limit it has today. + vi.mocked(serverRepo.findById).mockResolvedValue(makeServer({})); + vi.mocked(instanceRepo.findAll).mockResolvedValue([]); + + await service.reconcile('srv-1'); + + const spec = vi.mocked(orchestrator.createContainer).mock.calls[0]![0]; + expect(spec.memoryLimit).toBeUndefined(); + }); + }); + describe('reconcile', () => { it('starts instances when below desired replicas', async () => { vi.mocked(serverRepo.findById).mockResolvedValue(makeServer({ replicas: 2 })); diff --git a/src/mcpd/tests/mcp-server-repository-fields.test.ts b/src/mcpd/tests/mcp-server-repository-fields.test.ts index 843abaf..bd5f53a 100644 --- a/src/mcpd/tests/mcp-server-repository-fields.test.ts +++ b/src/mcpd/tests/mcp-server-repository-fields.test.ts @@ -45,4 +45,31 @@ describe('McpServerRepository field mapping', () => { } as never); expect(create.mock.calls[0]?.[0].data).toMatchObject({ secretDelivery: 'injector' }); }); + + it('persists memoryLimitMb on update', async () => { + const { spy, update } = prismaSpy(); + await new McpServerRepository(spy).update('id1', { memoryLimitMb: 2048 }); + expect(update.mock.calls[0]?.[0].data).toMatchObject({ memoryLimitMb: 2048 }); + }); + + it('persists memoryLimitMb on create, and nulls it when absent', async () => { + const { spy, create } = prismaSpy(); + await new McpServerRepository(spy).create({ + name: 'x', description: '', transport: 'STDIO', replicas: 1, env: [], volumes: [], + memoryLimitMb: 2048, + } as never); + expect(create.mock.calls[0]?.[0].data).toMatchObject({ memoryLimitMb: 2048 }); + + const second = prismaSpy(); + await new McpServerRepository(second.spy).create({ + name: 'y', description: '', transport: 'STDIO', replicas: 1, env: [], volumes: [], + } as never); + expect(second.create.mock.calls[0]?.[0].data).toMatchObject({ memoryLimitMb: null }); + }); + + it('leaves memoryLimitMb untouched on update when not supplied', async () => { + const { spy, update } = prismaSpy(); + await new McpServerRepository(spy).update('id1', { description: 'x' }); + expect(update.mock.calls[0]?.[0].data ?? {}).not.toHaveProperty('memoryLimitMb'); + }); });