fix(health): a passing tools/list is live, not healthy
Some checks failed
CI/CD / lint (pull_request) Successful in 1m16s
CI/CD / test (pull_request) Successful in 1m29s
CI/CD / typecheck (pull_request) Successful in 3m15s
CI/CD / smoke (pull_request) Failing after 2m0s
CI/CD / build (pull_request) Successful in 5m5s
CI/CD / publish (pull_request) Has been skipped
Some checks failed
CI/CD / lint (pull_request) Successful in 1m16s
CI/CD / test (pull_request) Successful in 1m29s
CI/CD / typecheck (pull_request) Successful in 3m15s
CI/CD / smoke (pull_request) Failing after 2m0s
CI/CD / build (pull_request) Successful in 5m5s
CI/CD / publish (pull_request) Has been skipped
`mcpctl get instances` showed all eight servers healthy while the UniFi one
had never once reached its controller. The default probe is `tools/list`,
which MCP servers answer from a static in-process table — no credentials, no
upstream, ~3ms. It cannot fail for any reason the user cares about, so it was
reporting `healthy` for every process that managed to start.
Split the two passes:
healthy — readiness: `tools/call` on `healthCheck.tool`. The upstream
answered, so the server can actually do its job.
live — liveness: `tools/list` only. Process up, upstream unverified.
`live` is now the default for any server without a `healthCheck.tool`. It is
not a warning; it is an admission that nothing is watching that server. Probe
events name which probe ran and which tool ("Readiness check (list_sites)
passed"), so the events log distinguishes the two after the fact.
Also:
- `healthCheck.tool` is optional now, so the timings can be tuned without
inventing a readiness probe.
- `create server --health-check-tool/-args/-interval/-timeout/
-failure-threshold`, per the rule that everything applyable is a create
flag. Merges over a `--from-template` healthCheck rather than replacing it.
- `describe instance` explains a `live` verdict instead of leaving it cryptic.
- create.ts held a raw NUL byte in a string literal, which made grep treat the
whole file as binary and silently skip it. Escaped as `\0`.
Verified against the live fleet: with readiness probes configured, my-grafana
went unhealthy (Grafana API 403) and my-node-red degraded (connect timeout to
a Tailscale address) — both had read healthy for months.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0114dg56YmVacyqhp5fitcTb
This commit is contained in:
185
src/mcplocal/tests/smoke/health-readiness.smoke.test.ts
Normal file
185
src/mcplocal/tests/smoke/health-readiness.smoke.test.ts
Normal file
@@ -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<T>(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<string, string> = { 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<string, unknown>;
|
||||
}
|
||||
|
||||
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<Server[]>('GET', '/api/v1/servers');
|
||||
expect(s.status, `GET /api/v1/servers returned ${s.status}`).toBe(200);
|
||||
servers = s.data;
|
||||
|
||||
const i = await mcpdRequest<Instance[]>('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<ProxyResult>('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<ProxyResult>('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([]);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user