From 859150770f4fe2fba02543f352d9f94003d9ecc1 Mon Sep 17 00:00:00 2001 From: Michal Date: Thu, 17 Sep 2026 00:24:18 +0100 Subject: [PATCH] feat(servers): per-server memory ceiling, because 512Mi OOMKills silently MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Server pods have always had a hardcoded 512Mi limit. That is right for a server that proxies an API and fatal for one that drives a browser: the `docs` server (docs-mcp-server, which scrapes with headless Chromium) idles at ~228Mi and crosses 512Mi within seconds of its first scrape. An OOMKill is the quietest failure we have. The kernel kills it, the pod restarts, the readiness probe passes, and the instance reads `healthy` again — while the six scrape jobs whose queue lived in memory are gone and the index has 17 pages in it. Nothing is logged, because the process never got to say anything. memoryLimitMb is declared per server, in MiB, and converted to bytes in the container spec. NULL keeps DEFAULT_MEMORY_LIMIT, so every existing server is bit-for-bit unchanged — a bigger default would have cost real memory on every node for servers that do not need it. mcpctl create server docs --memory-limit-mb 2048 --force Tests assert the two places a new column dies quietly: the repository's field-by-field mapping (a column not mapped there returns "patched" and changes nothing) and the spec built in instance.service. docs/reliability gains the OOMKill section — the `lastState.terminated.reason` check is what turns "it restarted again" into an answer. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JPjtnE6Gd343oRNtMU9Bcd --- completions/mcpctl.bash | 2 +- completions/mcpctl.fish | 1 + docs/reliability.md | 36 +++++++++++++++++++ src/cli/src/commands/apply.ts | 1 + src/cli/src/commands/create.ts | 4 +++ .../migration.sql | 9 +++++ src/db/prisma/schema.prisma | 7 ++++ .../src/repositories/mcp-server.repository.ts | 2 ++ src/mcpd/src/services/instance.service.ts | 10 ++++++ src/mcpd/src/validation/mcp-server.schema.ts | 5 +++ src/mcpd/tests/instance-service.test.ts | 34 +++++++++++++++++- .../mcp-server-repository-fields.test.ts | 27 ++++++++++++++ 12 files changed, 136 insertions(+), 2 deletions(-) create mode 100644 src/db/prisma/migrations/20260917000000_add_server_memory_limit/migration.sql 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'); + }); });