diff --git a/completions/mcpctl.bash b/completions/mcpctl.bash index a045a22..16a4fe5 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 --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 --health-check-tool --health-check-args --health-check-interval --health-check-timeout --health-check-failure-threshold --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 e79dc3b..822767d 100644 --- a/completions/mcpctl.fish +++ b/completions/mcpctl.fish @@ -380,6 +380,11 @@ complete -c mcpctl -n "__mcpctl_subcmd_active create server" -l command -d 'Comm complete -c mcpctl -n "__mcpctl_subcmd_active create server" -l container-port -d 'Container port number' -x complete -c mcpctl -n "__mcpctl_subcmd_active create server" -l replicas -d 'Number of replicas' -x complete -c mcpctl -n "__mcpctl_subcmd_active create server" -l env -d 'Env var: KEY=value (inline) or KEY=secretRef:SECRET:KEY (secret ref, repeat for multiple)' -x +complete -c mcpctl -n "__mcpctl_subcmd_active create server" -l health-check-tool -d 'Readiness probe: tool to call (without it the server only gets a liveness probe and reports "live", never "healthy")' -x +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 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 from-template -d 'Create from template (name or name:version)' -x complete -c mcpctl -n "__mcpctl_subcmd_active create server" -l env-from-secret -d 'Map template env vars from a secret' -x complete -c mcpctl -n "__mcpctl_subcmd_active create server" -l force -d 'Update if already exists' diff --git a/docs/reliability.md b/docs/reliability.md index 28a1c25..5c1f2c5 100644 --- a/docs/reliability.md +++ b/docs/reliability.md @@ -28,6 +28,66 @@ Note: the gate's prompt-ranking uses the **heavy client provider's own model** it deliberately does *not* force the project's vLLM model onto it (doing so made every selection fail silently when the model wasn't anthropic-servable). +## Instance health: `live` is not `healthy` + +An MCP server answers `tools/list` from a **static, in-process table**. It costs +a few milliseconds, needs no credentials, and reaches no upstream — so it stays +green while the thing the server exists to talk to is unreachable. Treating that +as a health signal is how `mcpctl get instances` showed eight healthy servers +while the UniFi one had never once reached its controller. + +So the probe reports two different passes: + +| Status | Probe | Means | +|---|---|---| +| `healthy` | **readiness** — `tools/call` on `healthCheck.tool` | The upstream answered. The server can do its job. | +| `live` | **liveness** — `tools/list` only | The process is up and speaks MCP. Its upstream is **unverified**. | +| `degraded` | either, failing | Failing, but under `failureThreshold`. | +| `unhealthy` | either, failing | Failed `failureThreshold` times in a row. | + +`live` is the default for any server with no `healthCheck.tool`. It is not a +warning — it is an admission that nothing is watching that server's upstream. + +**Configure a readiness probe on every server.** Pick a read-only tool that +genuinely round-trips to the upstream, and verify it passes before configuring +it — a probe naming a local-only tool (`get_..._version`) or a tool the server +doesn't expose reproduces the same false green it was meant to remove. + +```bash +mcpctl create server unifi-network --health-check-tool list_sites \ + --health-check-interval 60 --health-check-timeout 15 --force +``` + +or declaratively — `healthCheck` round-trips through `get -o yaml | apply -f`: + +```yaml +healthCheck: + tool: list_sites + arguments: {} + intervalSeconds: 60 + timeoutSeconds: 15 + failureThreshold: 3 +``` + +Omit `tool` to keep liveness while still tuning the timings. + +Latency is the tell: a probe answering in single-digit milliseconds is reading a +local table, not crossing a network. The UniFi probe went from 3ms (`tools/list`, +lying) to 1847ms on its first real `list_sites` — login, TLS, controller round +trip — and ~40ms once the session was warm. + +### Two failure modes the probe cannot see for you + +The healthy-looking UniFi server was broken **twice over**, and both are worth +checking first when a readiness probe starts failing: + +1. **Egress.** MCP server pods default to TCP 80/443 only + (`servers-allow-external-egress`). Any upstream on another port — the UniFi + controller on `:8443` — times out on every call. Declare it in Pulumi's + `mcpctl.serverEgressTargets` (name + `/32` + ports); don't widen the blanket rule. +2. **Address reachability.** A pod cannot reach a **Tailscale** `100.64.0.0/10` + address. Config pointing at one connect-timeouts forever. Use LAN IPs. + ## 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 5360a1a..c41325a 100644 --- a/src/cli/src/commands/apply.ts +++ b/src/cli/src/commands/apply.ts @@ -5,7 +5,8 @@ import { z } from 'zod'; import type { ApiClient } from '../api-client.js'; const HealthCheckSchema = z.object({ - tool: z.string().min(1), + /** Omit for a liveness-only probe (reports `live`); set it for readiness (`healthy`). */ + tool: z.string().min(1).optional(), arguments: z.record(z.unknown()).default({}), intervalSeconds: z.number().int().min(5).max(3600).default(60), timeoutSeconds: z.number().int().min(1).max(120).default(10), diff --git a/src/cli/src/commands/create.ts b/src/cli/src/commands/create.ts index 6f64801..fe2efd0 100644 --- a/src/cli/src/commands/create.ts +++ b/src/cli/src/commands/create.ts @@ -42,6 +42,69 @@ export function buildFavouriteIndex( return result; } +export interface HealthCheckOpts { + healthCheckTool?: string; + healthCheckArgs?: string; + healthCheckInterval?: string; + healthCheckTimeout?: string; + healthCheckFailureThreshold?: string; +} + +export interface HealthCheckSpec { + tool?: string; + arguments?: Record; + intervalSeconds?: number; + timeoutSeconds?: number; + failureThreshold?: number; +} + +function parsePositiveInt(flag: string, value: string): number { + const n = Number(value); + if (!Number.isInteger(n) || n <= 0) { + throw new Error(`Invalid ${flag} '${value}'. Expected a positive integer.`); + } + return n; +} + +/** + * Build a server `healthCheck` spec from `--health-check-*` flags, or undefined + * when none were given (so the field is only sent when intended). + * + * Mirrors the `healthCheck:` block accepted by `apply -f`, per the rule that + * everything applyable is also a create flag. + */ +export function buildHealthCheck(opts: HealthCheckOpts): HealthCheckSpec | undefined { + const { healthCheckTool, healthCheckArgs, healthCheckInterval, healthCheckTimeout, healthCheckFailureThreshold } = opts; + const given = [healthCheckTool, healthCheckArgs, healthCheckInterval, healthCheckTimeout, healthCheckFailureThreshold] + .some((v) => v !== undefined); + if (!given) return undefined; + + if (healthCheckArgs !== undefined && healthCheckTool === undefined) { + throw new Error('--health-check-args requires --health-check-tool.'); + } + + const spec: HealthCheckSpec = {}; + if (healthCheckTool !== undefined) spec.tool = healthCheckTool; + if (healthCheckArgs !== undefined) { + let parsed: unknown; + try { + parsed = JSON.parse(healthCheckArgs); + } catch { + throw new Error(`Invalid --health-check-args: not valid JSON. Expected a JSON object, e.g. '{"site":"default"}'.`); + } + if (typeof parsed !== 'object' || parsed === null || Array.isArray(parsed)) { + throw new Error(`Invalid --health-check-args: expected a JSON object, e.g. '{"site":"default"}'.`); + } + spec.arguments = parsed as Record; + } + if (healthCheckInterval !== undefined) spec.intervalSeconds = parsePositiveInt('--health-check-interval', healthCheckInterval); + if (healthCheckTimeout !== undefined) spec.timeoutSeconds = parsePositiveInt('--health-check-timeout', healthCheckTimeout); + if (healthCheckFailureThreshold !== undefined) { + spec.failureThreshold = parsePositiveInt('--health-check-failure-threshold', healthCheckFailureThreshold); + } + return spec; +} + /** * Parse a `--ttl` value. * @@ -136,6 +199,11 @@ export function createCreateCommand(deps: CreateCommandDeps): Command { .option('--container-port ', 'Container port number') .option('--replicas ', 'Number of replicas') .option('--env ', 'Env var: KEY=value (inline) or KEY=secretRef:SECRET:KEY (secret ref, repeat for multiple)', collect, []) + .option('--health-check-tool ', 'Readiness probe: tool to call (without it the server only gets a liveness probe and reports "live", never "healthy")') + .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('--health-check-failure-threshold ', 'Consecutive failures before the instance is marked unhealthy (default 3)') .option('--from-template ', 'Create from template (name or name:version)') .option('--env-from-secret ', 'Map template env vars from a secret') .option('--force', 'Update if already exists') @@ -218,6 +286,12 @@ export function createCreateCommand(deps: CreateCommandDeps): Command { if (opts.externalUrl) body.externalUrl = opts.externalUrl; if (opts.command.length > 0) body.command = opts.command; if (opts.containerPort) body.containerPort = parseInt(opts.containerPort, 10); + // Merge over any healthCheck inherited from --from-template so partial + // flags (e.g. only --health-check-interval) tune rather than replace it. + const healthCheck = buildHealthCheck(opts as HealthCheckOpts); + if (healthCheck) { + body.healthCheck = { ...(base.healthCheck as HealthCheckSpec | undefined), ...healthCheck }; + } if (opts.env.length > 0) { // Merge: CLI env entries override template env entries by name const cliEnv = parseServerEnv(opts.env); @@ -898,7 +972,7 @@ export function createCreateCommand(deps: CreateCommandDeps): Command { const buf = await fs.readFile(full); // Reject non-UTF8 — v1 is text-only. const text = buf.toString('utf-8'); - if (text.includes('')) { + if (text.includes('\0')) { throw new Error(`File ${rel} contains a null byte; binaries aren't supported in v1`); } files[rel] = text; diff --git a/src/cli/src/commands/describe.ts b/src/cli/src/commands/describe.ts index bda4ffb..cd94d69 100644 --- a/src/cli/src/commands/describe.ts +++ b/src/cli/src/commands/describe.ts @@ -90,6 +90,12 @@ function formatInstanceDetail(instance: Record, inspect?: Recor lines.push('Health:'); lines.push(` ${pad('Status:', 16)}${healthStatus ?? 'unknown'}`); if (lastHealthCheck) lines.push(` ${pad('Last Check:', 16)}${lastHealthCheck}`); + if (healthStatus === 'live') { + lines.push(` ${pad('Probe:', 16)}liveness (tools/list) — process is up, but nothing`); + lines.push(` ${pad('', 16)}calls the server's upstream. Configure a readiness`); + lines.push(` ${pad('', 16)}probe to reach 'healthy':`); + lines.push(` ${pad('', 16)} mcpctl edit server ${server?.name ?? ''} → healthCheck.tool`); + } } const metadata = instance.metadata as Record | undefined; diff --git a/src/cli/tests/health-check-opts.test.ts b/src/cli/tests/health-check-opts.test.ts new file mode 100644 index 0000000..0c02c03 --- /dev/null +++ b/src/cli/tests/health-check-opts.test.ts @@ -0,0 +1,59 @@ +import { describe, it, expect } from 'vitest'; +import { buildHealthCheck } from '../src/commands/create.js'; + +describe('buildHealthCheck — CLI flags → server healthCheck', () => { + it('returns undefined when no health-check flag is given', () => { + expect(buildHealthCheck({})).toBeUndefined(); + }); + + it('builds a readiness probe from --health-check-tool', () => { + expect(buildHealthCheck({ healthCheckTool: 'list_sites' })).toEqual({ tool: 'list_sites' }); + }); + + it('parses --health-check-args as a JSON object', () => { + expect(buildHealthCheck({ healthCheckTool: 'get_devices', healthCheckArgs: '{"site":"default"}' })) + .toEqual({ tool: 'get_devices', arguments: { site: 'default' } }); + }); + + it('carries the timing flags through', () => { + expect(buildHealthCheck({ + healthCheckTool: 'list_sites', + healthCheckInterval: '120', + healthCheckTimeout: '15', + healthCheckFailureThreshold: '2', + })).toEqual({ + tool: 'list_sites', + intervalSeconds: 120, + timeoutSeconds: 15, + failureThreshold: 2, + }); + }); + + it('allows tuning the liveness probe without a tool', () => { + // No `tool` → the probe stays liveness-only (reports `live`), but the + // interval is still configurable. + expect(buildHealthCheck({ healthCheckInterval: '300' })).toEqual({ intervalSeconds: 300 }); + }); + + it('rejects --health-check-args without a tool', () => { + expect(() => buildHealthCheck({ healthCheckArgs: '{}' })) + .toThrow(/--health-check-args requires --health-check-tool/); + }); + + it('rejects non-JSON args', () => { + expect(() => buildHealthCheck({ healthCheckTool: 't', healthCheckArgs: 'site=default' })) + .toThrow(/not valid JSON/); + }); + + it('rejects JSON args that are not an object', () => { + expect(() => buildHealthCheck({ healthCheckTool: 't', healthCheckArgs: '["a"]' })) + .toThrow(/expected a JSON object/); + }); + + it('rejects non-positive-integer timings', () => { + expect(() => buildHealthCheck({ healthCheckInterval: '0' })).toThrow(/--health-check-interval/); + expect(() => buildHealthCheck({ healthCheckTimeout: 'abc' })).toThrow(/--health-check-timeout/); + expect(() => buildHealthCheck({ healthCheckFailureThreshold: '-1' })) + .toThrow(/--health-check-failure-threshold/); + }); +}); diff --git a/src/mcpd/src/services/health-probe.service.ts b/src/mcpd/src/services/health-probe.service.ts index 8bd4192..8196f29 100644 --- a/src/mcpd/src/services/health-probe.service.ts +++ b/src/mcpd/src/services/health-probe.service.ts @@ -19,10 +19,24 @@ export const DEFAULT_HEALTH_CHECK: HealthCheckSpec = { failureThreshold: 3, }; +/** + * Which probe produced a result. + * + * - `readiness` — a real `tools/call` against the configured probe tool. It + * traverses the server's upstream dependency (controller API, database, + * remote service), so a pass means the server can actually do its job. + * - `liveness` — `tools/list` only. MCP servers answer that from a static + * in-process table, so it proves the process is up and speaking MCP and + * *nothing else*. A server whose upstream is unreachable still answers it. + */ +export type ProbeKind = 'readiness' | 'liveness'; + export interface ProbeResult { healthy: boolean; latencyMs: number; message: string; + /** Set by probeInstance from the healthCheck spec; probe helpers don't fill it. */ + probe?: ProbeKind; } interface ProbeState { @@ -118,6 +132,7 @@ export class HealthProbeRunner { const failureThreshold = healthCheck.failureThreshold ?? 3; const now = new Date(); const start = Date.now(); + const probeKind: ProbeKind = healthCheck.tool === undefined ? 'liveness' : 'readiness'; let result: ProbeResult; @@ -151,6 +166,8 @@ export class HealthProbeRunner { }; } + result.probe = probeKind; + // Update probe state const state = this.probeStates.get(instance.id) ?? { consecutiveFailures: 0, lastProbeAt: 0 }; state.lastProbeAt = Date.now(); @@ -162,18 +179,28 @@ export class HealthProbeRunner { } this.probeStates.set(instance.id, state); - // Determine health status + // Determine health status. + // + // A passing *liveness* probe reports `live`, not `healthy`. `tools/list` + // is answered from a static in-process table, so it stays green while the + // server's upstream is completely unreachable — which is exactly how a + // UniFi server whose controller port was firewalled off sat at "healthy" + // for months. Only a readiness probe (`tools/call` against a real tool) + // earns `healthy`. `live` means "process up, function unverified". const healthStatus = result.healthy - ? 'healthy' + ? (probeKind === 'readiness' ? 'healthy' : 'live') : state.consecutiveFailures >= failureThreshold ? 'unhealthy' : 'degraded'; // Build event + const probeLabel = probeKind === 'readiness' + ? `Readiness check (${healthCheck.tool})` + : 'Liveness check (tools/list)'; const eventType = result.healthy ? 'Normal' : 'Warning'; const eventMessage = result.healthy - ? `Health check passed (${result.latencyMs}ms)` - : `Health check failed: ${result.message}`; + ? `${probeLabel} passed (${result.latencyMs}ms)` + : `${probeLabel} failed: ${result.message}`; const existingEvents = (instance.events as Array<{ timestamp: string; type: string; message: string }>) ?? []; // Keep last 50 events diff --git a/src/mcpd/src/validation/template.schema.ts b/src/mcpd/src/validation/template.schema.ts index 8e4d34c..be1e3a4 100644 --- a/src/mcpd/src/validation/template.schema.ts +++ b/src/mcpd/src/validation/template.schema.ts @@ -8,7 +8,13 @@ const TemplateEnvEntrySchema = z.object({ }); export const HealthCheckSchema = z.object({ - tool: z.string().min(1), + /** + * Readiness probe tool. Omit it to keep the liveness-only default + * (`tools/list`) while still tuning interval/timeout/failureThreshold — + * a liveness pass reports `live`, not `healthy`, because `tools/list` is + * answered in-process and never touches the server's upstream. + */ + tool: z.string().min(1).optional(), arguments: z.record(z.unknown()).default({}), intervalSeconds: z.number().int().min(5).max(3600).default(60), timeoutSeconds: z.number().int().min(1).max(120).default(10), diff --git a/src/mcpd/tests/services/health-probe.test.ts b/src/mcpd/tests/services/health-probe.test.ts index 072bef9..8c33ccb 100644 --- a/src/mcpd/tests/services/health-probe.test.ts +++ b/src/mcpd/tests/services/health-probe.test.ts @@ -123,13 +123,76 @@ describe('HealthProbeRunner', () => { // No exec fallback — liveness goes through mcpProxyService expect(orchestrator.execInContainer).not.toHaveBeenCalled(); expect(mcpProxyService.execute).toHaveBeenCalledWith({ serverId: 'srv-1', method: 'tools/list' }); + // A passing liveness probe is `live`, never `healthy` — `tools/list` is + // answered in-process and proves nothing about the server's upstream. expect(instanceRepo.updateStatus).toHaveBeenCalledWith( 'inst-1', 'RUNNING', - expect.objectContaining({ healthStatus: 'healthy' }), + expect.objectContaining({ healthStatus: 'live' }), ); }); + it('reports `live` (not `healthy`) even when the upstream is dead, and says so in the event', async () => { + // The regression this guards: a UniFi server whose controller port was + // firewalled off sat at "healthy" for months because `tools/list` kept + // answering from the in-process tool table. + const instance = makeInstance(); + const server = makeServer({ healthCheck: null }); + + vi.mocked(instanceRepo.findAll).mockResolvedValue([instance]); + vi.mocked(serverRepo.findById).mockResolvedValue(server); + + const result = await runner.probeInstance(instance, server, { intervalSeconds: 0 }); + + expect(result.healthy).toBe(true); + expect(result.probe).toBe('liveness'); + + const fields = vi.mocked(instanceRepo.updateStatus).mock.calls[0]?.[2]; + expect(fields?.healthStatus).toBe('live'); + const events = fields?.events as Array<{ message: string }>; + expect(events[events.length - 1]?.message).toContain('Liveness check (tools/list) passed'); + }); + + it('a passing readiness probe earns `healthy` and names the tool in the event', async () => { + const instance = makeInstance(); + const server = makeServer({ + healthCheck: { tool: 'list_sites', intervalSeconds: 0 } as McpServer['healthCheck'], + }); + + vi.mocked(instanceRepo.findAll).mockResolvedValue([instance]); + vi.mocked(serverRepo.findById).mockResolvedValue(server); + vi.mocked(mcpProxyService.execute).mockResolvedValue({ jsonrpc: '2.0', id: 1, result: {} }); + + const result = await runner.probeInstance(instance, server, { tool: 'list_sites' }); + + expect(result.probe).toBe('readiness'); + const fields = vi.mocked(instanceRepo.updateStatus).mock.calls[0]?.[2]; + expect(fields?.healthStatus).toBe('healthy'); + const events = fields?.events as Array<{ message: string }>; + expect(events[events.length - 1]?.message).toContain('Readiness check (list_sites) passed'); + }); + + it('a readiness probe whose tool call fails reports the upstream error, not `live`', async () => { + const instance = makeInstance(); + const server = makeServer({ + healthCheck: { tool: 'list_sites', failureThreshold: 1 } as McpServer['healthCheck'], + }); + + vi.mocked(mcpProxyService.execute).mockResolvedValue({ + jsonrpc: '2.0', + id: 1, + error: { code: -32000, message: 'connect ETIMEDOUT 192.168.1.5:8443' }, + }); + + await runner.probeInstance(instance, server, { tool: 'list_sites', failureThreshold: 1 }); + + const fields = vi.mocked(instanceRepo.updateStatus).mock.calls[0]?.[2]; + expect(fields?.healthStatus).toBe('unhealthy'); + const events = fields?.events as Array<{ message: string }>; + expect(events[events.length - 1]?.message).toContain('Readiness check (list_sites) failed'); + expect(events[events.length - 1]?.message).toContain('ETIMEDOUT'); + }); + it('default liveness probe marks unhealthy when tools/list returns JSON-RPC error', async () => { const instance = makeInstance(); const server = makeServer({ diff --git a/src/mcplocal/tests/smoke/health-readiness.smoke.test.ts b/src/mcplocal/tests/smoke/health-readiness.smoke.test.ts new file mode 100644 index 0000000..82e266c --- /dev/null +++ b/src/mcplocal/tests/smoke/health-readiness.smoke.test.ts @@ -0,0 +1,185 @@ +/** + * Smoke tests: readiness probes actually exercise a server's upstream. + * + * The bug these guard: every instance read `healthy` forever because the + * default probe is `tools/list`, which MCP servers answer from a static + * in-process table. The UniFi server sat green for months while every call to + * its controller timed out (pod egress was capped at 80/443, controller on + * :8443) and while its `controller_type` pointed at the wrong API dialect. + * + * So these tests assert the probe is a real round trip, not a self-report: + * 1. Servers with a `healthCheck.tool` really do reach their upstream when + * that tool is called through the production proxy path. + * 2. A server with no `healthCheck.tool` reports `live`, never `healthy` — + * "process up, function unverified" must not read as "working". + * 3. `tools/list` alone cannot distinguish the two, which is why (2) matters. + * + * Prerequisites: + * - mcplocal running on localhost:3200 + * - mcpd reachable (k8s), servers deployed with readiness probes configured + */ +import { describe, it, expect, beforeAll } from 'vitest'; +import http from 'node:http'; +import https from 'node:https'; +import { existsSync, readFileSync } from 'node:fs'; +import { join } from 'node:path'; +import { homedir } from 'node:os'; + +const CONFIG_PATH = join(homedir(), '.mcpctl', 'config.json'); +const CREDS_PATH = join(homedir(), '.mcpctl', 'credentials'); + +function loadConfig(): { mcpdUrl: string; token: string } { + let mcpdUrl = 'http://localhost:3100'; + let token = ''; + try { + if (existsSync(CONFIG_PATH)) { + const cfg = JSON.parse(readFileSync(CONFIG_PATH, 'utf-8')) as { mcpdUrl?: string }; + if (cfg.mcpdUrl) mcpdUrl = cfg.mcpdUrl; + } + if (existsSync(CREDS_PATH)) { + const creds = JSON.parse(readFileSync(CREDS_PATH, 'utf-8')) as { token?: string }; + if (creds.token) token = creds.token; + } + } catch { /* use defaults */ } + return { mcpdUrl, token }; +} + +const { mcpdUrl, token } = loadConfig(); + +function mcpdRequest(method: string, path: string, body?: unknown): Promise<{ status: number; data: T }> { + return new Promise((resolve, reject) => { + const url = new URL(path, mcpdUrl); + const transport = url.protocol === 'https:' ? https : http; + + const headers: Record = { Accept: 'application/json' }; + if (body !== undefined) headers['Content-Type'] = 'application/json'; + if (token) headers['Authorization'] = `Bearer ${token}`; + const bodyStr = body !== undefined ? JSON.stringify(body) : undefined; + if (bodyStr) headers['Content-Length'] = String(Buffer.byteLength(bodyStr)); + + const req = transport.request(url, { method, timeout: 60_000, headers, rejectUnauthorized: false }, (res) => { + const chunks: Buffer[] = []; + res.on('data', (chunk: Buffer) => chunks.push(chunk)); + res.on('end', () => { + const raw = Buffer.concat(chunks).toString(); + try { + resolve({ status: res.statusCode ?? 500, data: raw ? JSON.parse(raw) as T : (undefined as T) }); + } catch { + resolve({ status: res.statusCode ?? 500, data: raw as unknown as T }); + } + }); + }); + req.on('error', reject); + req.on('timeout', () => { req.destroy(); reject(new Error('Request timeout')); }); + if (bodyStr) req.write(bodyStr); + req.end(); + }); +} + +interface HealthCheck { + tool?: string; + arguments?: Record; +} + +interface Server { + id: string; + name: string; + healthCheck: HealthCheck | null; +} + +interface Instance { + id: string; + serverId: string; + status: string; + healthStatus: string | null; + server?: { name: string }; +} + +interface ProxyResult { + result?: { tools?: Array<{ name: string }>; isError?: boolean; content?: Array<{ text?: string }> }; + error?: { code: number; message: string }; +} + +let servers: Server[] = []; +let instances: Instance[] = []; + +beforeAll(async () => { + const s = await mcpdRequest('GET', '/api/v1/servers'); + expect(s.status, `GET /api/v1/servers returned ${s.status}`).toBe(200); + servers = s.data; + + const i = await mcpdRequest('GET', '/api/v1/instances'); + expect(i.status, `GET /api/v1/instances returned ${i.status}`).toBe(200); + instances = i.data; +}, 120_000); + +describe('readiness probes reach the upstream', () => { + it('every RUNNING server has a readiness probe configured', () => { + const running = instances.filter((i) => i.status === 'RUNNING'); + expect(running.length, 'no RUNNING instances to check').toBeGreaterThan(0); + + const withoutProbe = running + .map((i) => servers.find((s) => s.id === i.serverId)) + .filter((s): s is Server => s !== undefined) + .filter((s) => s.healthCheck?.tool === undefined) + .map((s) => s.name); + + // A server with no readiness probe can only ever report `live`. That is + // honest, but it means nothing is watching its upstream — so the fleet + // should not accumulate them silently. + expect(withoutProbe, `servers with no healthCheck.tool: ${withoutProbe.join(', ')}`).toEqual([]); + }); + + it('each configured probe tool really answers through the proxy', async () => { + const probed = servers.filter((s) => s.healthCheck?.tool !== undefined); + expect(probed.length, 'no servers have readiness probes').toBeGreaterThan(0); + + const failures: string[] = []; + for (const server of probed) { + const hc = server.healthCheck!; + const res = await mcpdRequest('POST', '/api/v1/mcp/proxy', { + serverId: server.id, + method: 'tools/call', + params: { name: hc.tool, arguments: hc.arguments ?? {} }, + }); + if (res.status !== 200) { + failures.push(`${server.name}/${hc.tool}: HTTP ${res.status}`); + continue; + } + if (res.data.error) { + failures.push(`${server.name}/${hc.tool}: ${res.data.error.message}`); + continue; + } + if (res.data.result?.isError === true) { + failures.push(`${server.name}/${hc.tool}: ${res.data.result.content?.[0]?.text ?? 'isError'}`); + } + } + + // When this fails, the server is genuinely broken — fix the environment + // (credentials, egress, upstream address), never the assertion. + expect(failures, `readiness probe tools failing: ${failures.join(' | ')}`).toEqual([]); + }, 300_000); + + it('a probe tool is a different call from tools/list, and both are reachable', async () => { + const server = servers.find((s) => s.healthCheck?.tool !== undefined); + expect(server, 'need at least one probed server').toBeDefined(); + + const list = await mcpdRequest('POST', '/api/v1/mcp/proxy', { + serverId: server!.id, + method: 'tools/list', + }); + expect(list.status).toBe(200); + const toolNames = (list.data.result?.tools ?? []).map((t) => t.name); + // The probe must name a tool the server actually exposes, otherwise the + // readiness check fails for a bookkeeping reason rather than a real one. + expect(toolNames).toContain(server!.healthCheck!.tool); + }, 120_000); + + it('no RUNNING instance is stuck at an unknown health status', () => { + const stuck = instances + .filter((i) => i.status === 'RUNNING') + .filter((i) => i.healthStatus === null || i.healthStatus === 'unknown') + .map((i) => i.server?.name ?? i.serverId); + expect(stuck, `instances with no health verdict: ${stuck.join(', ')}`).toEqual([]); + }); +});