Compare commits
8 Commits
8c359902c7
...
fix/stdio-
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
5fb1154190 | ||
|
|
b022f322f0 | ||
|
|
c66502e590 | ||
|
|
740ce31469 | ||
| b6983f036d | |||
|
|
21aadf6d82 | ||
| 35d506df77 | |||
|
|
03350856ea |
@@ -7,7 +7,11 @@ Type=simple
|
||||
ExecStart=/usr/bin/mcpctl-local
|
||||
Restart=on-failure
|
||||
RestartSec=5
|
||||
Environment=MCPLOCAL_MCPD_URL=http://10.0.0.194:3100
|
||||
# mcpd now runs on Kubernetes behind this ingress. The previous default,
|
||||
# http://10.0.0.194:3100, is dead — a fresh install pointed at it and every
|
||||
# machine that worked did so via a hand-written drop-in. Override per host with:
|
||||
# systemctl --user edit mcplocal -> Environment=MCPLOCAL_MCPD_URL=...
|
||||
Environment=MCPLOCAL_MCPD_URL=https://mcpctl.ad.itaz.eu
|
||||
Environment=MCPLOCAL_HTTP_PORT=3200
|
||||
Environment=MCPLOCAL_HTTP_HOST=127.0.0.1
|
||||
|
||||
|
||||
@@ -187,14 +187,16 @@ trap - ERR
|
||||
|
||||
# ── 7. RPM + smoke ──
|
||||
say "7/7 Build/install CLI RPM + smoke tests"
|
||||
bash scripts/release.sh
|
||||
systemctl --user restart mcplocal && sleep 2
|
||||
if pnpm test:smoke > /tmp/deploy-smoke.log 2>&1; then
|
||||
grep -E "Tests |passed" /tmp/deploy-smoke.log | tail -2
|
||||
# release.sh already restarts mcplocal and runs the smoke suite against the
|
||||
# binary it just installed. This step used to restart and re-run it a second
|
||||
# time, which put two full suites inside a minute and tripped mcpd's rate
|
||||
# limiter: the second run failed with 429s and printed a false
|
||||
# "SMOKE TESTS FAILED — consider rollback" over a perfectly healthy deploy.
|
||||
# One run, one verdict.
|
||||
if bash scripts/release.sh; then
|
||||
say "Deploy complete — $TAG live. Rollback tag: $ROLLBACK_TAG"
|
||||
else
|
||||
tail -40 /tmp/deploy-smoke.log
|
||||
warn "SMOKE TESTS FAILED — system may be unhealthy. Consider rollback:"
|
||||
warn "RELEASE OR SMOKE TESTS FAILED — system may be unhealthy. Consider rollback:"
|
||||
rollback_recipe
|
||||
exit 1
|
||||
fi
|
||||
|
||||
@@ -511,6 +511,10 @@ async function main(): Promise<void> {
|
||||
const authService = new AuthService(prisma);
|
||||
const templateService = new TemplateService(templateRepo);
|
||||
const mcpProxyService = new McpProxyService(instanceRepo, serverRepo, orchestrator);
|
||||
// When syncStatus observes a container restart/replacement, the cached
|
||||
// STDIO pipe under that containerId is dead — evict it so the next call
|
||||
// redials (mcpctl#114). Setter injection, same as setInstanceService above.
|
||||
instanceService.setStdioInvalidator((cid) => mcpProxyService.removeClient(cid));
|
||||
const rbacDefinitionService = new RbacDefinitionService(rbacDefinitionRepo);
|
||||
const rbacService = new RbacService(rbacDefinitionRepo, prisma);
|
||||
const mcpTokenService = new McpTokenService(mcpTokenRepo, projectRepo, rbacDefinitionRepo, rbacService);
|
||||
@@ -969,6 +973,7 @@ async function main(): Promise<void> {
|
||||
healthProbeRunner.stop();
|
||||
secretBackendRotatorLoop.stop();
|
||||
gitBackup.stop();
|
||||
mcpProxyService.closeAll();
|
||||
await prisma.$disconnect();
|
||||
},
|
||||
});
|
||||
|
||||
@@ -265,6 +265,16 @@ export class DockerContainerManager implements McpOrchestrator {
|
||||
const stderr = new PassThrough();
|
||||
this.docker.modem.demuxStream(stream, stdout, stderr);
|
||||
|
||||
// demuxStream never propagates end/close/error to the demuxed streams, so
|
||||
// a dead container left consumers waiting on a silent pipe — same funnel
|
||||
// as the one-shot execInContainer path uses (mcpctl#114).
|
||||
const endStdout = () => {
|
||||
if (!stdout.destroyed && !stdout.writableEnded) stdout.end();
|
||||
};
|
||||
stream.on('end', endStdout);
|
||||
stream.on('close', endStdout);
|
||||
stream.on('error', endStdout);
|
||||
|
||||
return {
|
||||
stdout,
|
||||
write(data: string) {
|
||||
|
||||
@@ -193,6 +193,13 @@ export class HealthProbeRunner {
|
||||
? 'unhealthy'
|
||||
: 'degraded';
|
||||
|
||||
// Crossing the threshold means every probe rode the same cached STDIO
|
||||
// pipe — evict it once so the next probe/call redials instead of failing
|
||||
// forever against a connection the other side already dropped.
|
||||
if (!result.healthy && state.consecutiveFailures === failureThreshold && instance.containerId) {
|
||||
this.mcpProxyService?.removeClient(instance.containerId);
|
||||
}
|
||||
|
||||
// Build event
|
||||
const probeLabel = probeKind === 'readiness'
|
||||
? `Readiness check (${healthCheck.tool})`
|
||||
|
||||
@@ -32,6 +32,9 @@ interface RetryMetadata {
|
||||
attemptCount?: number;
|
||||
lastAttemptAt?: string;
|
||||
nextRetryAt?: string;
|
||||
/** containerStatuses[0].restartCount at last sync — a bump means every
|
||||
* cached STDIO pipe to this (unchanged) containerId is dead. */
|
||||
lastRestartCount?: number;
|
||||
[k: string]: unknown;
|
||||
}
|
||||
|
||||
@@ -52,6 +55,8 @@ export class InvalidStateError extends Error {
|
||||
}
|
||||
|
||||
export class InstanceService {
|
||||
private stdioInvalidator?: (containerId: string) => void;
|
||||
|
||||
constructor(
|
||||
private instanceRepo: IMcpInstanceRepository,
|
||||
private serverRepo: IMcpServerRepository,
|
||||
@@ -59,6 +64,25 @@ export class InstanceService {
|
||||
private secretResolver?: SecretResolver,
|
||||
) {}
|
||||
|
||||
/**
|
||||
* Hook for evicting cached STDIO clients (McpProxyService.removeClient).
|
||||
* Setter injection, matching serverService.setInstanceService in main.ts —
|
||||
* McpProxyService is constructed after this service and already imports
|
||||
* from this file, so a constructor arg would be a circular import.
|
||||
*/
|
||||
setStdioInvalidator(fn: (containerId: string) => void): void {
|
||||
this.stdioInvalidator = fn;
|
||||
}
|
||||
|
||||
private invalidateStdio(containerId: string | null | undefined): void {
|
||||
if (!containerId) return;
|
||||
try {
|
||||
this.stdioInvalidator?.(containerId);
|
||||
} catch {
|
||||
/* best-effort */
|
||||
}
|
||||
}
|
||||
|
||||
async list(serverId?: string): Promise<McpInstance[]> {
|
||||
return this.instanceRepo.findAll(serverId);
|
||||
}
|
||||
@@ -71,37 +95,96 @@ export class InstanceService {
|
||||
|
||||
/**
|
||||
* Sync instance statuses with actual container state.
|
||||
* Detects crashed/stopped containers and marks them ERROR.
|
||||
*
|
||||
* Beyond marking crashed containers ERROR, this is the recovery path for
|
||||
* mcpctl#114: ERROR rows whose pod came back are re-adopted (the pod has
|
||||
* restartPolicy Always, so kubelet restarts the container in place and the
|
||||
* row must follow it back), and an in-place restart under an unchanged
|
||||
* containerId — visible only as a restartCount bump — invalidates any
|
||||
* cached STDIO pipe, which is dead by definition.
|
||||
*
|
||||
* Every metadata write here MERGES via readRetryMeta: updateStatus replaces
|
||||
* the JSON column wholesale, and clobbering nextRetryAt is what used to
|
||||
* make ERROR rows instantly dueForRetry and hot-loop against a 409.
|
||||
*/
|
||||
async syncStatus(): Promise<void> {
|
||||
const instances = await this.instanceRepo.findAll();
|
||||
for (const inst of instances) {
|
||||
if ((inst.status === 'RUNNING' || inst.status === 'STARTING') && inst.containerId) {
|
||||
try {
|
||||
const info = await this.orchestrator.inspectContainer(inst.containerId);
|
||||
if (!inst.containerId) continue;
|
||||
if (inst.status !== 'RUNNING' && inst.status !== 'STARTING' && inst.status !== 'ERROR') {
|
||||
continue;
|
||||
}
|
||||
|
||||
if (info.state === 'stopped' || info.state === 'error') {
|
||||
// Container died — get last logs for error context
|
||||
let errorMsg = `Container ${info.state}`;
|
||||
try {
|
||||
const logs = await this.orchestrator.getContainerLogs(inst.containerId, { tail: 5 });
|
||||
const lastLog = (logs.stdout || logs.stderr).trim().split('\n').pop();
|
||||
if (lastLog) errorMsg = lastLog;
|
||||
} catch { /* best-effort */ }
|
||||
await this.instanceRepo.updateStatus(inst.id, 'ERROR', {
|
||||
metadata: { error: errorMsg },
|
||||
});
|
||||
} else if (info.state === 'starting' && inst.status === 'RUNNING') {
|
||||
// Pod went back to starting (e.g. CrashLoopBackOff restart)
|
||||
await this.instanceRepo.updateStatus(inst.id, 'STARTING', {});
|
||||
} else if (info.state === 'running' && inst.status === 'STARTING') {
|
||||
// Pod became ready — promote to RUNNING
|
||||
await this.instanceRepo.updateStatus(inst.id, 'RUNNING', {});
|
||||
}
|
||||
} catch {
|
||||
// Container gone entirely
|
||||
let info: ContainerInfo;
|
||||
try {
|
||||
info = await this.orchestrator.inspectContainer(inst.containerId);
|
||||
} catch {
|
||||
// Container gone entirely. ERROR rows with a missing pod stay as they
|
||||
// are — the retry/backoff path owns recreating them.
|
||||
if (inst.status !== 'ERROR') {
|
||||
await this.instanceRepo.updateStatus(inst.id, 'ERROR', {
|
||||
metadata: { error: 'Container not found' },
|
||||
metadata: { ...readRetryMeta(inst), error: 'Container not found' },
|
||||
});
|
||||
this.invalidateStdio(inst.containerId);
|
||||
}
|
||||
continue;
|
||||
}
|
||||
|
||||
const meta = readRetryMeta(inst);
|
||||
|
||||
if (inst.status === 'ERROR') {
|
||||
// The pod outlived the ERROR verdict (kubelet restarted the container
|
||||
// in place). Re-adopt instead of leaving the row stuck forever.
|
||||
if (info.state === 'running') {
|
||||
const { error: _e, attemptCount: _a, lastAttemptAt: _l, nextRetryAt: _n, ...rest } = meta;
|
||||
await this.instanceRepo.updateStatus(inst.id, 'RUNNING', {
|
||||
metadata: { ...rest, lastRestartCount: info.restartCount ?? 0 },
|
||||
});
|
||||
this.invalidateStdio(inst.containerId);
|
||||
} else if (info.state === 'starting') {
|
||||
// Keep retry metadata until it is actually running.
|
||||
await this.instanceRepo.updateStatus(inst.id, 'STARTING', { metadata: meta });
|
||||
this.invalidateStdio(inst.containerId);
|
||||
}
|
||||
// stopped/error: leave for the backoff/retry path.
|
||||
continue;
|
||||
}
|
||||
|
||||
if (info.state === 'stopped' || info.state === 'error') {
|
||||
// Container died — get last logs for error context
|
||||
let errorMsg = `Container ${info.state}`;
|
||||
try {
|
||||
const logs = await this.orchestrator.getContainerLogs(inst.containerId, { tail: 5 });
|
||||
const lastLog = (logs.stdout || logs.stderr).trim().split('\n').pop();
|
||||
if (lastLog) errorMsg = lastLog;
|
||||
} catch { /* best-effort */ }
|
||||
await this.instanceRepo.updateStatus(inst.id, 'ERROR', {
|
||||
metadata: { ...meta, error: errorMsg },
|
||||
});
|
||||
this.invalidateStdio(inst.containerId);
|
||||
} else if (info.state === 'starting' && inst.status === 'RUNNING') {
|
||||
// Pod went back to starting (e.g. CrashLoopBackOff restart)
|
||||
await this.instanceRepo.updateStatus(inst.id, 'STARTING', { metadata: meta });
|
||||
this.invalidateStdio(inst.containerId);
|
||||
} else if (info.state === 'running' && inst.status === 'STARTING') {
|
||||
// Pod became ready — promote to RUNNING and clear retry state.
|
||||
const { error: _e, attemptCount: _a, lastAttemptAt: _l, nextRetryAt: _n, ...rest } = meta;
|
||||
await this.instanceRepo.updateStatus(inst.id, 'RUNNING', {
|
||||
metadata: { ...rest, lastRestartCount: info.restartCount ?? 0 },
|
||||
});
|
||||
// A fresh/restarted pod under this name invalidates any cached pipe.
|
||||
this.invalidateStdio(inst.containerId);
|
||||
} else if (info.state === 'running' && info.restartCount !== undefined) {
|
||||
if (typeof meta.lastRestartCount === 'number' && info.restartCount > meta.lastRestartCount) {
|
||||
// In-place restart between polls: same pod name, dead pipes.
|
||||
await this.instanceRepo.updateStatus(inst.id, 'RUNNING', {
|
||||
metadata: { ...meta, lastRestartCount: info.restartCount },
|
||||
});
|
||||
this.invalidateStdio(inst.containerId);
|
||||
} else if (meta.lastRestartCount === undefined) {
|
||||
// First sighting: record the baseline without invalidating.
|
||||
await this.instanceRepo.updateStatus(inst.id, 'RUNNING', {
|
||||
metadata: { ...meta, lastRestartCount: info.restartCount },
|
||||
});
|
||||
}
|
||||
}
|
||||
@@ -232,6 +315,7 @@ export class InstanceService {
|
||||
} catch {
|
||||
// Container may already be gone
|
||||
}
|
||||
this.invalidateStdio(instance.containerId);
|
||||
}
|
||||
|
||||
await this.instanceRepo.delete(id);
|
||||
@@ -256,6 +340,7 @@ export class InstanceService {
|
||||
} catch {
|
||||
// best-effort
|
||||
}
|
||||
this.invalidateStdio(inst.containerId);
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -489,6 +574,7 @@ export class InstanceService {
|
||||
try {
|
||||
await this.orchestrator.removeContainer(instance.containerId, true);
|
||||
} catch { /* best-effort */ }
|
||||
this.invalidateStdio(instance.containerId);
|
||||
}
|
||||
await this.instanceRepo.delete(instance.id);
|
||||
}
|
||||
|
||||
@@ -81,6 +81,18 @@ function podToContainerInfo(pod: V1Pod): ContainerInfo {
|
||||
info.port = ports[0].containerPort;
|
||||
}
|
||||
|
||||
// Restart visibility: a container that crashes and restarts IN PLACE keeps
|
||||
// the same pod name and reports state=running again — restartCount is the
|
||||
// only durable evidence, and syncStatus uses it to invalidate stale STDIO
|
||||
// pipes (mcpctl#114).
|
||||
const cs = pod.status?.containerStatuses?.[0];
|
||||
if (cs?.restartCount !== undefined) {
|
||||
info.restartCount = cs.restartCount;
|
||||
}
|
||||
if (cs?.state?.running?.startedAt) {
|
||||
info.startedAt = new Date(cs.state.running.startedAt as unknown as string);
|
||||
}
|
||||
|
||||
return info;
|
||||
}
|
||||
|
||||
@@ -116,10 +128,39 @@ export class KubernetesOrchestrator implements McpOrchestrator {
|
||||
}
|
||||
|
||||
const manifest = generatePodSpec(spec, this.namespace);
|
||||
const pod = await this.client.core.createNamespacedPod({
|
||||
namespace: this.namespace,
|
||||
body: manifest as V1Pod,
|
||||
});
|
||||
let pod;
|
||||
try {
|
||||
pod = await this.client.core.createNamespacedPod({
|
||||
namespace: this.namespace,
|
||||
body: manifest as V1Pod,
|
||||
});
|
||||
} catch (err) {
|
||||
if (httpStatusOf(err) !== 409) throw err;
|
||||
// AlreadyExists: a retry raced a pod that is still there. If it is
|
||||
// alive, ADOPT it — recreating under the same name can never succeed
|
||||
// and used to loop the instance in ERROR forever (mcpctl#114). Only a
|
||||
// genuinely dead pod is replaced.
|
||||
const podName = (manifest as V1Pod).metadata!.name!;
|
||||
const existing = await this.inspectContainer(podName);
|
||||
if (existing.state === 'running' || existing.state === 'starting') {
|
||||
return existing;
|
||||
}
|
||||
await this.removeContainer(podName, true);
|
||||
// deleteNamespacedPod returns before the pod is gone (grace period);
|
||||
// recreating while it is Terminating would 409 again. Bounded wait.
|
||||
for (let i = 0; i < 15; i++) {
|
||||
await new Promise((resolve) => setTimeout(resolve, 1000));
|
||||
try {
|
||||
await this.inspectContainer(podName);
|
||||
} catch {
|
||||
break; // 404 — pod is gone
|
||||
}
|
||||
}
|
||||
pod = await this.client.core.createNamespacedPod({
|
||||
namespace: this.namespace,
|
||||
body: manifest as V1Pod,
|
||||
});
|
||||
}
|
||||
|
||||
// Wait briefly for pod to start scheduling
|
||||
await new Promise((resolve) => setTimeout(resolve, 500));
|
||||
@@ -276,6 +317,17 @@ export class KubernetesOrchestrator implements McpOrchestrator {
|
||||
// Wait for WebSocket connection to establish
|
||||
const ws = await wsPromise;
|
||||
|
||||
// client-node's WebSocketHandler only ends stdout on an explicit
|
||||
// CloseStream/Status frame from the apiserver. An abnormal socket death
|
||||
// (container OOMKilled, node reboot) emits neither — without these
|
||||
// handlers the PassThrough stays open and the consumer never learns the
|
||||
// pipe is dead (mcpctl#114).
|
||||
const endStdout = () => {
|
||||
if (!stdout.destroyed && !stdout.writableEnded) stdout.end();
|
||||
};
|
||||
ws.on('close', endStdout);
|
||||
ws.on('error', endStdout);
|
||||
|
||||
return {
|
||||
stdout,
|
||||
write(data: string) {
|
||||
@@ -316,6 +368,13 @@ export class KubernetesOrchestrator implements McpOrchestrator {
|
||||
false, // tty
|
||||
);
|
||||
|
||||
// Same abnormal-death funnel as execInteractive — see comment there.
|
||||
const endStdout = () => {
|
||||
if (!stdout.destroyed && !stdout.writableEnded) stdout.end();
|
||||
};
|
||||
ws.on('close', endStdout);
|
||||
ws.on('error', endStdout);
|
||||
|
||||
return {
|
||||
stdout,
|
||||
write(data: string) {
|
||||
|
||||
@@ -180,20 +180,28 @@ export class McpProxyService {
|
||||
} catch (err) {
|
||||
this.removeClient(instance.containerId);
|
||||
// Fall back to one-shot exec when we have a command to run.
|
||||
// Attach mode has no equivalent one-shot fallback — surface the error.
|
||||
if (mode.kind === 'exec') {
|
||||
return sendViaStdio(this.orchestrator, instance.containerId, packageName, method, params, 120_000, command, runtime);
|
||||
}
|
||||
const detail = formatError(err);
|
||||
console.error(`[mcp-proxy] attach to ${instance.containerId} failed:`, err);
|
||||
return {
|
||||
jsonrpc: '2.0',
|
||||
id: 1,
|
||||
error: {
|
||||
code: -32000,
|
||||
message: `STDIO attach to '${instance.containerId}' failed: ${detail}`,
|
||||
},
|
||||
};
|
||||
// Attach mode has no one-shot equivalent, but the failure is usually
|
||||
// a stale pipe from an in-place container restart — retry once
|
||||
// through a fresh client (which redials) before surfacing the error,
|
||||
// so the FIRST call after a detected death succeeds (mcpctl#114).
|
||||
try {
|
||||
return await this.sendViaPersistentStdio(instance.containerId, mode, method, params);
|
||||
} catch (retryErr) {
|
||||
this.removeClient(instance.containerId);
|
||||
const detail = formatError(retryErr);
|
||||
console.error(`[mcp-proxy] attach to ${instance.containerId} failed (after retry):`, retryErr);
|
||||
return {
|
||||
jsonrpc: '2.0',
|
||||
id: 1,
|
||||
error: {
|
||||
code: -32000,
|
||||
message: `STDIO attach to '${instance.containerId}' failed: ${detail}`,
|
||||
},
|
||||
};
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -69,6 +69,14 @@ export interface ContainerInfo {
|
||||
/** Container IP on the first non-default network (for internal communication) */
|
||||
ip?: string;
|
||||
createdAt: Date;
|
||||
/**
|
||||
* Times the container restarted in place (k8s containerStatuses[0]).
|
||||
* A bump with an unchanged containerId means every cached STDIO pipe to it
|
||||
* is dead — syncStatus uses this to invalidate them (mcpctl#114).
|
||||
*/
|
||||
restartCount?: number;
|
||||
/** When the current container process started (k8s state.running.startedAt). */
|
||||
startedAt?: Date;
|
||||
}
|
||||
|
||||
export interface ContainerLogs {
|
||||
|
||||
@@ -121,15 +121,15 @@ export class PersistentStdioClient {
|
||||
this.processBuffer();
|
||||
});
|
||||
|
||||
exec.stdout.on('end', () => {
|
||||
this.initialized = false;
|
||||
this.exec = null;
|
||||
for (const [, pending] of this.pendingRequests) {
|
||||
clearTimeout(pending.timer);
|
||||
pending.reject(new Error('STDIO process exited'));
|
||||
}
|
||||
this.pendingRequests.clear();
|
||||
});
|
||||
// All three events funnel into the same teardown: 'end' is the graceful
|
||||
// path, but an abnormal websocket death may only surface as 'close' or
|
||||
// 'error' on the stream — before this, isConnected stayed true and every
|
||||
// request rode out the full timeout against a dead pipe (mcpctl#114).
|
||||
exec.stdout.on('end', () => this.teardown(exec, 'STDIO process exited'));
|
||||
exec.stdout.on('close', () => this.teardown(exec, 'STDIO stream closed'));
|
||||
exec.stdout.on('error', (err: Error) =>
|
||||
this.teardown(exec, `STDIO stream error: ${err.message}`),
|
||||
);
|
||||
|
||||
// Run MCP init handshake
|
||||
const initId = this.nextId++;
|
||||
@@ -174,6 +174,23 @@ export class PersistentStdioClient {
|
||||
this.initialized = true;
|
||||
}
|
||||
|
||||
/**
|
||||
* Tear down a dead session: reject in-flight requests and mark the client
|
||||
* disconnected so the next send() redials via ensureReady(). The identity
|
||||
* guard is load-bearing: after a reconnect, a LATE 'close'/'end' from the
|
||||
* previous session's stream must not clobber the new session.
|
||||
*/
|
||||
private teardown(exec: InteractiveExec, reason: string): void {
|
||||
if (this.exec !== exec) return;
|
||||
this.initialized = false;
|
||||
this.exec = null;
|
||||
for (const [, pending] of this.pendingRequests) {
|
||||
clearTimeout(pending.timer);
|
||||
pending.reject(new Error(reason));
|
||||
}
|
||||
this.pendingRequests.clear();
|
||||
}
|
||||
|
||||
private write(msg: Record<string, unknown>): void {
|
||||
if (!this.exec) throw new Error('Not connected');
|
||||
this.exec.write(JSON.stringify(msg) + '\n');
|
||||
|
||||
@@ -511,3 +511,130 @@ describe('InstanceService', () => {
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
// ── syncStatus (mcpctl#114) — first-ever coverage of this method ──
|
||||
|
||||
describe('syncStatus', () => {
|
||||
let instanceRepo: IMcpInstanceRepository;
|
||||
let serverRepo: IMcpServerRepository;
|
||||
let orchestrator: McpOrchestrator;
|
||||
let service: InstanceService;
|
||||
let invalidator: ReturnType<typeof vi.fn>;
|
||||
|
||||
function inspect(info: Partial<{ state: string; restartCount: number }>) {
|
||||
vi.mocked(orchestrator.inspectContainer).mockResolvedValue({
|
||||
containerId: 'ctr-abc',
|
||||
name: 'test',
|
||||
state: (info.state ?? 'running') as 'running',
|
||||
createdAt: new Date(),
|
||||
...(info.restartCount !== undefined ? { restartCount: info.restartCount } : {}),
|
||||
});
|
||||
}
|
||||
|
||||
beforeEach(() => {
|
||||
instanceRepo = mockInstanceRepo();
|
||||
serverRepo = mockServerRepo();
|
||||
orchestrator = mockOrchestrator();
|
||||
service = new InstanceService(instanceRepo, serverRepo, orchestrator);
|
||||
invalidator = vi.fn();
|
||||
service.setStdioInvalidator(invalidator);
|
||||
});
|
||||
|
||||
it('marks a stopped container ERROR and MERGES retry metadata instead of clobbering it', async () => {
|
||||
const future = new Date(Date.now() + 60_000).toISOString();
|
||||
vi.mocked(instanceRepo.findAll).mockResolvedValue([
|
||||
makeInstance({ status: 'RUNNING', metadata: { attemptCount: 3, nextRetryAt: future } }),
|
||||
]);
|
||||
inspect({ state: 'stopped' });
|
||||
|
||||
await service.syncStatus();
|
||||
|
||||
const [, status, fields] = vi.mocked(instanceRepo.updateStatus).mock.calls[0]!;
|
||||
expect(status).toBe('ERROR');
|
||||
const meta = fields!.metadata as Record<string, unknown>;
|
||||
expect(meta['error']).toBe('log output'); // last log line
|
||||
expect(meta['attemptCount']).toBe(3); // preserved — was clobbered before
|
||||
expect(meta['nextRetryAt']).toBe(future); // preserved — hot-loop killer
|
||||
expect(invalidator).toHaveBeenCalledWith('ctr-abc');
|
||||
});
|
||||
|
||||
it('recovers an ERROR instance whose pod is running again (the stuck state)', async () => {
|
||||
vi.mocked(instanceRepo.findAll).mockResolvedValue([
|
||||
makeInstance({
|
||||
status: 'ERROR',
|
||||
metadata: { error: 'Container stopped', attemptCount: 4, nextRetryAt: 'x', lastAttemptAt: 'y' },
|
||||
}),
|
||||
]);
|
||||
inspect({ state: 'running', restartCount: 2 });
|
||||
|
||||
await service.syncStatus();
|
||||
|
||||
const [, status, fields] = vi.mocked(instanceRepo.updateStatus).mock.calls[0]!;
|
||||
expect(status).toBe('RUNNING');
|
||||
const meta = fields!.metadata as Record<string, unknown>;
|
||||
expect(meta['error']).toBeUndefined();
|
||||
expect(meta['attemptCount']).toBeUndefined();
|
||||
expect(meta['nextRetryAt']).toBeUndefined();
|
||||
expect(meta['lastRestartCount']).toBe(2);
|
||||
expect(invalidator).toHaveBeenCalledWith('ctr-abc');
|
||||
});
|
||||
|
||||
it('leaves an ERROR instance alone when its pod is genuinely gone', async () => {
|
||||
vi.mocked(instanceRepo.findAll).mockResolvedValue([
|
||||
makeInstance({ status: 'ERROR', metadata: { error: 'x' } }),
|
||||
]);
|
||||
vi.mocked(orchestrator.inspectContainer).mockRejectedValue(new Error('not found'));
|
||||
|
||||
await service.syncStatus();
|
||||
|
||||
expect(instanceRepo.updateStatus).not.toHaveBeenCalled();
|
||||
expect(invalidator).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('detects an in-place restart via restartCount and invalidates the stdio pipe', async () => {
|
||||
vi.mocked(instanceRepo.findAll).mockResolvedValue([
|
||||
makeInstance({ status: 'RUNNING', metadata: { lastRestartCount: 1 } }),
|
||||
]);
|
||||
inspect({ state: 'running', restartCount: 2 });
|
||||
|
||||
await service.syncStatus();
|
||||
|
||||
const [, status, fields] = vi.mocked(instanceRepo.updateStatus).mock.calls[0]!;
|
||||
expect(status).toBe('RUNNING');
|
||||
expect((fields!.metadata as Record<string, unknown>)['lastRestartCount']).toBe(2);
|
||||
expect(invalidator).toHaveBeenCalledWith('ctr-abc');
|
||||
});
|
||||
|
||||
it('baselines a first-seen restartCount without invalidating', async () => {
|
||||
vi.mocked(instanceRepo.findAll).mockResolvedValue([
|
||||
makeInstance({ status: 'RUNNING', metadata: {} }),
|
||||
]);
|
||||
inspect({ state: 'running', restartCount: 0 });
|
||||
|
||||
await service.syncStatus();
|
||||
|
||||
const [, , fields] = vi.mocked(instanceRepo.updateStatus).mock.calls[0]!;
|
||||
expect((fields!.metadata as Record<string, unknown>)['lastRestartCount']).toBe(0);
|
||||
expect(invalidator).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('does nothing for a steady RUNNING instance with an unchanged restartCount', async () => {
|
||||
vi.mocked(instanceRepo.findAll).mockResolvedValue([
|
||||
makeInstance({ status: 'RUNNING', metadata: { lastRestartCount: 2 } }),
|
||||
]);
|
||||
inspect({ state: 'running', restartCount: 2 });
|
||||
|
||||
await service.syncStatus();
|
||||
|
||||
expect(instanceRepo.updateStatus).not.toHaveBeenCalled();
|
||||
expect(invalidator).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('remove() evicts the cached stdio client', async () => {
|
||||
vi.mocked(instanceRepo.findById).mockResolvedValue(makeInstance({ status: 'RUNNING' }));
|
||||
|
||||
await service.remove('inst-1');
|
||||
|
||||
expect(invalidator).toHaveBeenCalledWith('ctr-abc');
|
||||
});
|
||||
});
|
||||
|
||||
@@ -63,12 +63,22 @@ vi.mock('@kubernetes/client-node', () => {
|
||||
makeApiClient = vi.fn(() => mockCore);
|
||||
}
|
||||
|
||||
// Track the live Exec/Attach instances so tests can program their vi.fn()s
|
||||
// (each K8sOfficialClient constructs fresh ones).
|
||||
const instances: { exec?: { exec: ReturnType<typeof vi.fn> }; attach?: { attach: ReturnType<typeof vi.fn> } } = {};
|
||||
|
||||
class MockExec {
|
||||
exec = vi.fn();
|
||||
constructor() {
|
||||
instances.exec = this;
|
||||
}
|
||||
}
|
||||
|
||||
class MockAttach {
|
||||
attach = vi.fn();
|
||||
constructor() {
|
||||
instances.attach = this;
|
||||
}
|
||||
}
|
||||
|
||||
class MockLog {
|
||||
@@ -82,7 +92,7 @@ vi.mock('@kubernetes/client-node', () => {
|
||||
Attach: MockAttach,
|
||||
Log: MockLog,
|
||||
// Export test helpers
|
||||
__testHelpers: { setHandler, getHandler, clearHandlers, mockCore },
|
||||
__testHelpers: { setHandler, getHandler, clearHandlers, mockCore, instances },
|
||||
};
|
||||
});
|
||||
|
||||
@@ -379,3 +389,134 @@ describe('httpStatusOf', () => {
|
||||
expect(httpStatusOf({ code: 'ECONNREFUSED' })).toBeUndefined();
|
||||
});
|
||||
});
|
||||
|
||||
// ── mcpctl#114: interactive-session death signals, restartCount, 409-adopt ──
|
||||
|
||||
import { EventEmitter } from 'node:events';
|
||||
|
||||
function makeFakeWs(): EventEmitter & { close: ReturnType<typeof vi.fn> } {
|
||||
const ws = new EventEmitter() as EventEmitter & { close: ReturnType<typeof vi.fn> };
|
||||
ws.close = vi.fn();
|
||||
return ws;
|
||||
}
|
||||
|
||||
const podRestarted = {
|
||||
...podRunning,
|
||||
status: {
|
||||
...podRunning.status,
|
||||
containerStatuses: [{
|
||||
state: { running: { startedAt: '2026-01-02T00:00:00Z' } },
|
||||
restartCount: 3,
|
||||
lastState: { terminated: { reason: 'OOMKilled', exitCode: 137 } },
|
||||
}],
|
||||
},
|
||||
};
|
||||
|
||||
const podTerminated = {
|
||||
...podRunning,
|
||||
status: {
|
||||
phase: 'Failed',
|
||||
containerStatuses: [{
|
||||
state: { terminated: { reason: 'Error', exitCode: 1 } },
|
||||
restartCount: 1,
|
||||
}],
|
||||
},
|
||||
};
|
||||
|
||||
describe('KubernetesOrchestrator interactive sessions (mcpctl#114)', () => {
|
||||
let orch: KubernetesOrchestrator;
|
||||
|
||||
beforeEach(() => {
|
||||
clearHandlers();
|
||||
vi.clearAllMocks();
|
||||
orch = new KubernetesOrchestrator({ serversNamespace: 'mcpctl-servers' });
|
||||
});
|
||||
|
||||
it("execInteractive: ws 'close' ends stdout so the consumer sees the death", async () => {
|
||||
setHandler('readNamespacedPod:my-server', podRunning);
|
||||
const ws = makeFakeWs();
|
||||
k8sMock.__testHelpers.instances.exec!.exec.mockResolvedValue(ws);
|
||||
|
||||
const iexec = await orch.execInteractive!('my-server', ['node', 'index.js']);
|
||||
iexec.stdout.resume(); // PassThrough only emits 'end' once it is being read
|
||||
const ended = new Promise<void>((resolve) => iexec.stdout.on('end', resolve));
|
||||
ws.emit('close');
|
||||
await expect(ended).resolves.toBeUndefined();
|
||||
});
|
||||
|
||||
it("execInteractive: ws 'error' also ends stdout, and close() closes the ws", async () => {
|
||||
setHandler('readNamespacedPod:my-server', podRunning);
|
||||
const ws = makeFakeWs();
|
||||
k8sMock.__testHelpers.instances.exec!.exec.mockResolvedValue(ws);
|
||||
|
||||
const iexec = await orch.execInteractive!('my-server', ['node']);
|
||||
iexec.stdout.resume();
|
||||
const ended = new Promise<void>((resolve) => iexec.stdout.on('end', resolve));
|
||||
ws.emit('error', new Error('abnormal closure'));
|
||||
await expect(ended).resolves.toBeUndefined();
|
||||
|
||||
iexec.close();
|
||||
expect(ws.close).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("attachInteractive: ws 'close' ends stdout (first-ever attach coverage)", async () => {
|
||||
setHandler('readNamespacedPod:my-server', podRunning);
|
||||
const ws = makeFakeWs();
|
||||
k8sMock.__testHelpers.instances.attach!.attach.mockResolvedValue(ws);
|
||||
|
||||
const iexec = await orch.attachInteractive!('my-server');
|
||||
iexec.stdout.resume();
|
||||
const ended = new Promise<void>((resolve) => iexec.stdout.on('end', resolve));
|
||||
ws.emit('close');
|
||||
await expect(ended).resolves.toBeUndefined();
|
||||
expect(k8sMock.__testHelpers.instances.attach!.attach).toHaveBeenCalledOnce();
|
||||
});
|
||||
|
||||
it('inspectContainer surfaces restartCount and startedAt', async () => {
|
||||
setHandler('readNamespacedPod:my-server', podRestarted);
|
||||
const info = await orch.inspectContainer('my-server');
|
||||
expect(info.state).toBe('running');
|
||||
expect(info.restartCount).toBe(3);
|
||||
expect(info.startedAt).toEqual(new Date('2026-01-02T00:00:00Z'));
|
||||
});
|
||||
|
||||
it('createContainer ADOPTS an alive pod on 409 instead of throwing', async () => {
|
||||
setHandler('readNamespace:mcpctl-servers', {});
|
||||
setHandler('createNamespacedPod', undefined, { code: 409 });
|
||||
setHandler('readNamespacedPod:my-server', podRestarted);
|
||||
|
||||
const info = await orch.createContainer(testSpec);
|
||||
expect(info.containerId).toBe('my-server');
|
||||
expect(info.state).toBe('running');
|
||||
expect(mockCore.deleteNamespacedPod).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('createContainer replaces a genuinely dead pod on 409', async () => {
|
||||
setHandler('readNamespace:mcpctl-servers', {});
|
||||
setHandler('createNamespacedPod', undefined, { code: 409 });
|
||||
setHandler('readNamespacedPod:my-server', podTerminated);
|
||||
setHandler('deleteNamespacedPod:my-server', {});
|
||||
|
||||
const promise = orch.createContainer(testSpec);
|
||||
// Give it a tick to inspect + delete, then flip the world: pod is gone,
|
||||
// create succeeds.
|
||||
await new Promise((r) => setTimeout(r, 50));
|
||||
// The gone-wait loop polls once per second: serve 404 so it breaks, then
|
||||
// restore the pod before the post-create inspect (~500ms later) runs.
|
||||
setHandler('readNamespacedPod:my-server', undefined, { code: 404 });
|
||||
setHandler('createNamespacedPod', podRunning);
|
||||
setTimeout(() => setHandler('readNamespacedPod:my-server', podRunning), 1200);
|
||||
|
||||
const info = await promise;
|
||||
expect(info.state).toBe('running');
|
||||
expect(mockCore.deleteNamespacedPod).toHaveBeenCalled();
|
||||
expect(mockCore.createNamespacedPod).toHaveBeenCalledTimes(2);
|
||||
});
|
||||
|
||||
it('createContainer still throws on non-409 errors', async () => {
|
||||
setHandler('readNamespace:mcpctl-servers', {});
|
||||
setHandler('createNamespacedPod', undefined, { code: 500 });
|
||||
await expect(orch.createContainer(testSpec)).rejects.toBeTruthy();
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
@@ -725,3 +725,103 @@ describe('MCP server full flow', () => {
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
// ── mcpctl#114: attach server survives an in-place container restart ──
|
||||
|
||||
import { PassThrough } from 'node:stream';
|
||||
import type { InteractiveExec } from '../src/services/orchestrator.js';
|
||||
|
||||
/**
|
||||
* A scripted MCP server on the other end of an attach session: answers
|
||||
* initialize and every request with a result carrying its session number,
|
||||
* so the test can prove which session served which call.
|
||||
*/
|
||||
function makeScriptedAttach(session: number): InteractiveExec {
|
||||
const stdout = new PassThrough();
|
||||
return {
|
||||
stdout,
|
||||
write(data: string) {
|
||||
for (const line of data.split('\n')) {
|
||||
if (!line.trim()) continue;
|
||||
const msg = JSON.parse(line) as { id?: number; method?: string };
|
||||
if (msg.id === undefined) continue; // notifications
|
||||
const result = msg.method === 'initialize'
|
||||
? { capabilities: {} }
|
||||
: { ok: true, session };
|
||||
stdout.write(JSON.stringify({ jsonrpc: '2.0', id: msg.id, result }) + '\n');
|
||||
}
|
||||
},
|
||||
close() {
|
||||
stdout.destroy();
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
describe('attach-mode STDIO restart recovery (mcpctl#114)', () => {
|
||||
it('survives an in-place container restart: syncStatus invalidates, next call rides a new session', async () => {
|
||||
const serverRepo = createInMemoryServerRepo();
|
||||
const instanceRepo = createInMemoryInstanceRepo();
|
||||
|
||||
const server = await serverRepo.create({
|
||||
name: 'gitea',
|
||||
transport: 'STDIO',
|
||||
dockerImage: 'ghcr.io/gitea-mcp:latest',
|
||||
replicas: 1,
|
||||
} as never);
|
||||
const instance = await instanceRepo.create({
|
||||
serverId: server.id,
|
||||
containerId: 'pod-gitea',
|
||||
status: 'RUNNING',
|
||||
metadata: { lastRestartCount: 0 },
|
||||
} as never);
|
||||
|
||||
let restartCount = 0;
|
||||
const sessions: InteractiveExec[] = [];
|
||||
const orchestrator = {
|
||||
ping: vi.fn(async () => true),
|
||||
pullImage: vi.fn(async () => {}),
|
||||
createContainer: vi.fn(),
|
||||
stopContainer: vi.fn(async () => {}),
|
||||
removeContainer: vi.fn(async () => {}),
|
||||
inspectContainer: vi.fn(async () => ({
|
||||
containerId: 'pod-gitea',
|
||||
name: 'pod-gitea',
|
||||
state: 'running' as const,
|
||||
createdAt: new Date(),
|
||||
restartCount,
|
||||
})),
|
||||
getContainerLogs: vi.fn(async () => ({ stdout: '', stderr: '' })),
|
||||
execInContainer: vi.fn(),
|
||||
attachInteractive: vi.fn(async () => {
|
||||
const s = makeScriptedAttach(sessions.length + 1);
|
||||
sessions.push(s);
|
||||
return s;
|
||||
}),
|
||||
} as unknown as McpOrchestrator;
|
||||
|
||||
const proxyService = new McpProxyService(instanceRepo, serverRepo, orchestrator);
|
||||
const instanceService = new InstanceService(instanceRepo, serverRepo, orchestrator);
|
||||
instanceService.setStdioInvalidator((cid) => proxyService.removeClient(cid));
|
||||
|
||||
// 1. A call round-trips on session 1.
|
||||
const first = await proxyService.execute({ serverId: server.id, method: 'tools/list' });
|
||||
expect((first.result as { session: number }).session).toBe(1);
|
||||
|
||||
// 2. The container crashes and kubelet restarts it in place: same pod
|
||||
// name, restartCount bumps, the old session's pipe dies.
|
||||
sessions[0]!.stdout.end();
|
||||
restartCount = 1;
|
||||
|
||||
// 3. One reconcile tick detects the bump, keeps the row RUNNING, and
|
||||
// evicts the stale client.
|
||||
await instanceService.syncStatus();
|
||||
const after = await instanceRepo.findById(instance.id);
|
||||
expect(after!.status).toBe('RUNNING');
|
||||
expect((after!.metadata as { lastRestartCount: number }).lastRestartCount).toBe(1);
|
||||
|
||||
// 4. The next call succeeds first-try on a fresh attach session.
|
||||
const second = await proxyService.execute({ serverId: server.id, method: 'tools/list' });
|
||||
expect((second.result as { session: number }).session).toBe(2);
|
||||
expect(orchestrator.attachInteractive).toHaveBeenCalledTimes(2);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -109,3 +109,110 @@ describe('PersistentStdioClient', () => {
|
||||
await expect(client.send('tools/list')).rejects.toThrow(/interactive exec/i);
|
||||
});
|
||||
});
|
||||
|
||||
// Drive a fake session through the init handshake so send() can round-trip.
|
||||
async function completeHandshake(fake: ReturnType<typeof makeFakeExec>): Promise<void> {
|
||||
await new Promise((r) => setTimeout(r, 10));
|
||||
const init = JSON.parse(fake.written[0]!);
|
||||
fake.emit({ jsonrpc: '2.0', id: init.id, result: { capabilities: {} } });
|
||||
await new Promise((r) => setTimeout(r, 150));
|
||||
}
|
||||
|
||||
describe('PersistentStdioClient teardown & reconnect (mcpctl#114)', () => {
|
||||
it("stdout 'end' rejects in-flight requests and the next send() redials", async () => {
|
||||
const fakes = [makeFakeExec(), makeFakeExec()];
|
||||
let call = 0;
|
||||
const execInteractive = vi.fn(async () => fakes[call++]!.iexec);
|
||||
const client = new PersistentStdioClient(
|
||||
makeOrchestrator({ execInteractive }),
|
||||
'c1',
|
||||
{ kind: 'exec', command: ['node'] },
|
||||
);
|
||||
|
||||
const first = client.send('tools/list');
|
||||
await completeHandshake(fakes[0]!);
|
||||
// Kill the session while tools/list is in flight.
|
||||
fakes[0]!.iexec.stdout.end();
|
||||
await expect(first).rejects.toThrow('STDIO process exited');
|
||||
expect(client.isConnected).toBe(false);
|
||||
|
||||
// Next send must redial through a fresh interactive session.
|
||||
const second = client.send('tools/list');
|
||||
await completeHandshake(fakes[1]!);
|
||||
const req = JSON.parse(fakes[1]!.written[2]!);
|
||||
fakes[1]!.emit({ jsonrpc: '2.0', id: req.id, result: { tools: [] } });
|
||||
const res = await second;
|
||||
expect(res.result).toEqual({ tools: [] });
|
||||
expect(execInteractive).toHaveBeenCalledTimes(2);
|
||||
client.close();
|
||||
});
|
||||
|
||||
it("stdout 'error' tears down with the error message", async () => {
|
||||
const fake = makeFakeExec();
|
||||
const client = new PersistentStdioClient(
|
||||
makeOrchestrator({ execInteractive: vi.fn(async () => fake.iexec) }),
|
||||
'c1',
|
||||
{ kind: 'exec', command: ['node'] },
|
||||
);
|
||||
const pending = client.send('tools/list');
|
||||
await completeHandshake(fake);
|
||||
fake.iexec.stdout.emit('error', new Error('socket hang up'));
|
||||
await expect(pending).rejects.toThrow('STDIO stream error: socket hang up');
|
||||
expect(client.isConnected).toBe(false);
|
||||
client.close();
|
||||
});
|
||||
|
||||
it("a LATE 'end' from the previous session does not clobber the reconnected one", async () => {
|
||||
const fakes = [makeFakeExec(), makeFakeExec()];
|
||||
let call = 0;
|
||||
const client = new PersistentStdioClient(
|
||||
makeOrchestrator({ attachInteractive: vi.fn(async () => fakes[call++]!.iexec) }),
|
||||
'c1',
|
||||
{ kind: 'attach' },
|
||||
);
|
||||
|
||||
const first = client.send('tools/list');
|
||||
await completeHandshake(fakes[0]!);
|
||||
fakes[0]!.iexec.stdout.end();
|
||||
await expect(first).rejects.toThrow('STDIO process exited');
|
||||
|
||||
// Reconnect on a fresh session.
|
||||
const second = client.send('tools/list');
|
||||
await completeHandshake(fakes[1]!);
|
||||
|
||||
// The old session's stream fires a stale 'close' AFTER the reconnect —
|
||||
// the identity guard must ignore it.
|
||||
fakes[0]!.iexec.stdout.emit('close');
|
||||
expect(client.isConnected).toBe(true);
|
||||
|
||||
const req = JSON.parse(fakes[1]!.written[2]!);
|
||||
fakes[1]!.emit({ jsonrpc: '2.0', id: req.id, result: { tools: [{ name: 't' }] } });
|
||||
const res = await second;
|
||||
expect((res.result as { tools: unknown[] }).tools).toHaveLength(1);
|
||||
client.close();
|
||||
});
|
||||
|
||||
it('attach mode also redials after teardown', async () => {
|
||||
const fakes = [makeFakeExec(), makeFakeExec()];
|
||||
let call = 0;
|
||||
const attachInteractive = vi.fn(async () => fakes[call++]!.iexec);
|
||||
const client = new PersistentStdioClient(
|
||||
makeOrchestrator({ attachInteractive }),
|
||||
'c-gitea',
|
||||
{ kind: 'attach' },
|
||||
);
|
||||
|
||||
const first = client.send('tools/list');
|
||||
await completeHandshake(fakes[0]!);
|
||||
fakes[0]!.iexec.stdout.emit('close');
|
||||
await expect(first).rejects.toThrow('STDIO stream closed');
|
||||
|
||||
const second = client.send('tools/list');
|
||||
await completeHandshake(fakes[1]!);
|
||||
const req = JSON.parse(fakes[1]!.written[2]!);
|
||||
fakes[1]!.emit({ jsonrpc: '2.0', id: req.id, result: { tools: [] } });
|
||||
await expect(second).resolves.toMatchObject({ result: { tools: [] } });
|
||||
expect(attachInteractive).toHaveBeenCalledTimes(2);
|
||||
client.close();
|
||||
});
|
||||
});
|
||||
|
||||
@@ -544,3 +544,32 @@ describe('HealthProbeRunner', () => {
|
||||
expect(result.message).toBe('ECONNREFUSED 10.0.0.1:3000');
|
||||
});
|
||||
});
|
||||
|
||||
describe('HealthProbeRunner stale-pipe eviction (mcpctl#114)', () => {
|
||||
it('evicts the cached stdio client exactly once, when failures cross the threshold', async () => {
|
||||
const instanceRepo = mockInstanceRepo();
|
||||
const serverRepo = mockServerRepo();
|
||||
const orchestrator = mockOrchestrator();
|
||||
const mcpProxyService = mockMcpProxyService();
|
||||
const runner = new HealthProbeRunner(instanceRepo, serverRepo, orchestrator, undefined, mcpProxyService);
|
||||
|
||||
const instance = makeInstance();
|
||||
// intervalSeconds: 0 so every tick actually probes; failureThreshold: 3.
|
||||
const server = makeServer({
|
||||
healthCheck: { tool: 'list_datasources', arguments: {}, intervalSeconds: 0, timeoutSeconds: 10, failureThreshold: 3 },
|
||||
} as Partial<McpServer>);
|
||||
vi.mocked(instanceRepo.findAll).mockResolvedValue([instance]);
|
||||
vi.mocked(serverRepo.findById).mockResolvedValue(server);
|
||||
vi.mocked(mcpProxyService.execute).mockRejectedValue(new Error('pipe is dead'));
|
||||
|
||||
await runner.tick(); // failure 1 — degraded
|
||||
await runner.tick(); // failure 2 — degraded
|
||||
expect(mcpProxyService.removeClient).not.toHaveBeenCalled();
|
||||
|
||||
await runner.tick(); // failure 3 — crosses threshold → evict once
|
||||
expect(mcpProxyService.removeClient).toHaveBeenCalledExactlyOnceWith('container-abc');
|
||||
|
||||
await runner.tick(); // failure 4 — already past threshold, no re-evict
|
||||
expect(mcpProxyService.removeClient).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -1,5 +1,5 @@
|
||||
import type { McpdClient } from './http/mcpd-client.js';
|
||||
import { DISCOVERY_TIMEOUT_MS } from './http/mcpd-client.js';
|
||||
import { DISCOVERY_TIMEOUT_MS, TOOLCALL_TIMEOUT_MS } from './http/mcpd-client.js';
|
||||
import type { McpRouter } from './router.js';
|
||||
import { McpdUpstream } from './upstream/mcpd.js';
|
||||
|
||||
@@ -152,6 +152,10 @@ function syncUpstreams(router: McpRouter, mcpdClient: McpdClient, servers: McpdS
|
||||
// unreachable upstream cannot stall session init for the full tool-call window.
|
||||
const discoveryClient = mcpdClient.withTimeout(DISCOVERY_TIMEOUT_MS);
|
||||
|
||||
// Everything else an upstream receives is a proxied tools/call, which can
|
||||
// legitimately run past the 30s default (browser solvers, PDF extraction).
|
||||
const toolClient = mcpdClient.withTimeout(TOOLCALL_TIMEOUT_MS);
|
||||
|
||||
// Remove stale upstreams
|
||||
const currentNames = new Set(router.getUpstreamNames());
|
||||
const serverNames = new Set(servers.map((s) => s.name));
|
||||
@@ -164,7 +168,7 @@ function syncUpstreams(router: McpRouter, mcpdClient: McpdClient, servers: McpdS
|
||||
// Add/update upstreams for each server
|
||||
for (const server of servers) {
|
||||
if (!currentNames.has(server.name)) {
|
||||
const upstream = new McpdUpstream(server.id, server.name, mcpdClient, server.description, discoveryClient);
|
||||
const upstream = new McpdUpstream(server.id, server.name, toolClient, server.description, discoveryClient);
|
||||
router.addUpstream(upstream);
|
||||
}
|
||||
registered.push(server.name);
|
||||
|
||||
@@ -62,6 +62,17 @@ export const LONG_RUNNING_TIMEOUT_MS = Number(process.env['MCPLOCAL_LONG_TIMEOUT
|
||||
*/
|
||||
export const DISCOVERY_TIMEOUT_MS = Number(process.env['MCPLOCAL_DISCOVERY_TIMEOUT_MS']) || 8_000;
|
||||
|
||||
/**
|
||||
* Budget for proxied tools/call. The 30s DEFAULT_TIMEOUT_MS is a guaranteed
|
||||
* failure for tools that legitimately run long — web_url_read through a
|
||||
* browser solver, a large PDF extraction, a slow retail site — all of which
|
||||
* died as `mcpd proxy error: mcpd did not respond within 30000ms` while mcpd
|
||||
* was still working. Distinct from LONG_RUNNING_TIMEOUT_MS (chat/inference,
|
||||
* minutes) and DISCOVERY_TIMEOUT_MS (list calls, must stay short so a dead
|
||||
* upstream cannot stall session init). Override via `MCPLOCAL_TOOLCALL_TIMEOUT_MS`.
|
||||
*/
|
||||
export const TOOLCALL_TIMEOUT_MS = Number(process.env['MCPLOCAL_TOOLCALL_TIMEOUT_MS']) || 120_000;
|
||||
|
||||
export class McpdClient {
|
||||
private readonly baseUrl: string;
|
||||
private readonly token: string;
|
||||
|
||||
@@ -176,9 +176,13 @@ export function registerProjectMcpEndpoint(app: FastifyInstance, mcpdClient: Mcp
|
||||
chain.push(createAgentsPlugin());
|
||||
router.setPlugin(composePlugins(chain));
|
||||
|
||||
// Fetch project instructions and set on router
|
||||
// Fetch project instructions and set on router. Must ride the CALLER's
|
||||
// token (requestClient) like every other downstream call here: in HTTP
|
||||
// mode mcpdClient's own token is empty, mcpd answers 401, and the catch
|
||||
// below swallowed it — every session initialized with no instructions
|
||||
// while nothing looked broken (#113).
|
||||
try {
|
||||
const instructions = await mcpdClient.get<{ prompt: string; servers: Array<{ name: string; description: string }> }>(
|
||||
const instructions = await requestClient.get<{ prompt: string; servers: Array<{ name: string; description: string }> }>(
|
||||
`/api/v1/projects/${encodeURIComponent(projectName)}/instructions`,
|
||||
);
|
||||
const parts: string[] = [];
|
||||
|
||||
@@ -7,20 +7,37 @@
|
||||
* - sectionStore management
|
||||
*
|
||||
* This plugin handles:
|
||||
* 1. onToolCallBefore: intercept section drill-down requests (_resultId + _section params)
|
||||
* 2. onToolCallAfter: run tool results through the proxymodel pipeline
|
||||
* 1. onToolsList: declare the drill-down params on every tool it may paginate
|
||||
* 2. onToolCallBefore: intercept section drill-down requests (_resultId + _section params)
|
||||
* 3. onToolCallAfter: run tool results through the proxymodel pipeline
|
||||
*/
|
||||
import type { JsonRpcRequest, JsonRpcResponse } from '../../types.js';
|
||||
import type { Section } from '../types.js';
|
||||
import type { Section, ToolDefinition } from '../types.js';
|
||||
import type { ProxyModelPlugin, PluginSessionContext } from '../plugin.js';
|
||||
|
||||
const SECTION_STORE_TTL_MS = 300_000; // 5 minutes
|
||||
|
||||
/**
|
||||
* Tools owned by the gate plugin. Their calls are intercepted in
|
||||
* onToolCallBefore and returned before onToolCallAfter ever runs, so their
|
||||
* results never paginate and they must not advertise drill-down params.
|
||||
*/
|
||||
const GATE_TOOLS = new Set(['begin_session', 'read_prompts', 'propose_prompt', 'propose_skill']);
|
||||
|
||||
export function createContentPipelinePlugin(): ProxyModelPlugin {
|
||||
return {
|
||||
name: 'content-pipeline',
|
||||
description: 'Content transformation pipeline: paginate, section-split, summarize tool results.',
|
||||
|
||||
async onToolsList(tools): Promise<ToolDefinition[]> {
|
||||
// The drill-down params are part of this plugin's contract with the
|
||||
// client, so they belong in the advertised schema. Without them a client
|
||||
// that validates arguments against inputSchema (and upstreams that set
|
||||
// `additionalProperties: false`, e.g. unifi-network) can never send
|
||||
// _resultId/_section, which makes every paginated result unreadable.
|
||||
return tools.map(withDrillDownParams);
|
||||
},
|
||||
|
||||
async onToolCallBefore(_toolName, args, request, ctx) {
|
||||
// Intercept section drill-down requests
|
||||
const resultId = args['_resultId'] as string | undefined;
|
||||
@@ -30,6 +47,13 @@ export function createContentPipelinePlugin(): ProxyModelPlugin {
|
||||
return handleSectionDrillDown(request, resultId, section, ctx);
|
||||
}
|
||||
|
||||
// _resultId without _section: the caller wants the cached result but did
|
||||
// not name a section. Re-show the table of contents instead of
|
||||
// forwarding _resultId upstream, where it is an unknown argument.
|
||||
if (resultId !== undefined && resultId !== '') {
|
||||
return handleSectionListing(request, resultId, ctx);
|
||||
}
|
||||
|
||||
return null;
|
||||
},
|
||||
|
||||
@@ -50,8 +74,20 @@ export function createContentPipelinePlugin(): ProxyModelPlugin {
|
||||
if (response.error) return response;
|
||||
|
||||
// Extract text content from the response
|
||||
const raw = extractTextContent(response);
|
||||
if (!raw || raw.length <= 2000) return response;
|
||||
const extracted = extractTextContent(response);
|
||||
if (extracted === null) return response;
|
||||
|
||||
// Collapse nested MCP envelopes first. A server that proxies another MCP
|
||||
// server (unifi-network does) returns the inner result wrapped in its own
|
||||
// content/structuredContent pair, so the same payload arrives two or
|
||||
// three times over. Unwrapping halves it, which keeps many results under
|
||||
// the pagination threshold entirely.
|
||||
const raw = unwrapNestedEnvelopes(extracted);
|
||||
const unwrapped = raw !== extracted;
|
||||
|
||||
if (raw.length <= 2000) {
|
||||
return unwrapped ? textResponse(response.id, raw) : response;
|
||||
}
|
||||
|
||||
try {
|
||||
const result = await ctx.processContent(toolName, raw, 'toolResult');
|
||||
@@ -61,7 +97,7 @@ export function createContentPipelinePlugin(): ProxyModelPlugin {
|
||||
const resultId = `pm-${Date.now().toString(36)}`;
|
||||
storeSections(ctx, resultId, result.sections);
|
||||
|
||||
const text = `${result.content}\n\n_resultId: ${resultId} — use _resultId and _section parameters to drill into a section.`;
|
||||
const text = `${result.content}\n\n${drillDownInstruction(toolName, resultId, result.sections)}`;
|
||||
return {
|
||||
jsonrpc: '2.0',
|
||||
id: response.id,
|
||||
@@ -71,21 +107,174 @@ export function createContentPipelinePlugin(): ProxyModelPlugin {
|
||||
|
||||
// Pipeline ran but no sections — return processed content if it changed
|
||||
if (result.content !== raw) {
|
||||
return {
|
||||
jsonrpc: '2.0',
|
||||
id: response.id,
|
||||
result: { content: [{ type: 'text', text: result.content }] },
|
||||
};
|
||||
return textResponse(response.id, result.content);
|
||||
}
|
||||
} catch {
|
||||
// Pipeline failed — return original response
|
||||
// Pipeline failed — fall through, but keep the unwrap
|
||||
}
|
||||
|
||||
if (unwrapped) return textResponse(response.id, raw);
|
||||
|
||||
return response;
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
/**
|
||||
* Build the drill-down instruction appended to a sectioned result.
|
||||
*
|
||||
* Names both parameters exactly as the schema declares them and shows a real
|
||||
* section id, so a model can copy the call rather than guess at it. Guessing
|
||||
* used to mean sending a bare `section`, which fell through to the upstream
|
||||
* and re-paginated with a fresh _resultId — an unbounded loop.
|
||||
*/
|
||||
function drillDownInstruction(toolName: string, resultId: string, sections: Section[]): string {
|
||||
const example = sections[0]?.id ?? 'page-1';
|
||||
return `To read one, call ${toolName} again with _resultId="${resultId}" and _section="${example}". `
|
||||
+ 'Both parameters are required together; pass no other arguments.';
|
||||
}
|
||||
|
||||
/**
|
||||
* Declare the drill-down params on a tool's advertised input schema.
|
||||
*
|
||||
* Only `properties` is extended: `additionalProperties: false` stays as the
|
||||
* upstream set it, because a property listed in `properties` is allowed by
|
||||
* that keyword. Keeping it false preserves the upstream's typo protection.
|
||||
*/
|
||||
function withDrillDownParams(tool: ToolDefinition): ToolDefinition {
|
||||
if (GATE_TOOLS.has(tool.name)) return tool;
|
||||
|
||||
const schema = tool.inputSchema;
|
||||
if (typeof schema !== 'object' || schema === null || Array.isArray(schema)) return tool;
|
||||
|
||||
const s = schema as Record<string, unknown>;
|
||||
if (s['type'] !== undefined && s['type'] !== 'object') return tool;
|
||||
|
||||
const existing = s['properties'];
|
||||
const props: Record<string, unknown> =
|
||||
typeof existing === 'object' && existing !== null && !Array.isArray(existing)
|
||||
? { ...(existing as Record<string, unknown>) }
|
||||
: {};
|
||||
|
||||
if ('_resultId' in props || '_section' in props) return tool;
|
||||
|
||||
props['_resultId'] = {
|
||||
type: 'string',
|
||||
description:
|
||||
'Set only when re-reading a large result this tool already returned. '
|
||||
+ 'Pass the _resultId from that result together with _section.',
|
||||
};
|
||||
props['_section'] = {
|
||||
type: 'string',
|
||||
description:
|
||||
'Section id to read (e.g. "page-1"), from the table of contents of a '
|
||||
+ 'previous large result. Requires _resultId.',
|
||||
};
|
||||
|
||||
return { ...tool, inputSchema: { ...s, properties: props } };
|
||||
}
|
||||
|
||||
/** Build a single-text-part tool result. */
|
||||
function textResponse(id: JsonRpcResponse['id'], text: string): JsonRpcResponse {
|
||||
return { jsonrpc: '2.0', id, result: { content: [{ type: 'text', text }] } };
|
||||
}
|
||||
|
||||
/** Maximum envelope layers to peel. Guards against pathological nesting. */
|
||||
const MAX_UNWRAP_DEPTH = 5;
|
||||
|
||||
/**
|
||||
* Collapse nested MCP result envelopes.
|
||||
*
|
||||
* A server that fronts another MCP server hands back the inner result still
|
||||
* wrapped: `{ ...meta, content: [{ type: 'text', text: "<inner JSON>" }],
|
||||
* structuredContent: <same inner value> }`. The payload is then present twice
|
||||
* — once escaped inside `content`, once parsed in `structuredContent` — and
|
||||
* the whole thing is itself a string inside the outer result. unifi-network's
|
||||
* get_clients arrives at 136K this way.
|
||||
*
|
||||
* A layer is peeled only when it carries nothing the inner value doesn't
|
||||
* already have: every sibling key must reappear in the inner value with an
|
||||
* equal value, and `structuredContent` (if present) must equal it too. That
|
||||
* makes the collapse lossless; anything else is left alone.
|
||||
*/
|
||||
function unwrapNestedEnvelopes(text: string): string {
|
||||
let current: unknown;
|
||||
try {
|
||||
current = JSON.parse(text);
|
||||
} catch {
|
||||
return text; // Not JSON — nothing to unwrap.
|
||||
}
|
||||
|
||||
let peeled = false;
|
||||
for (let depth = 0; depth < MAX_UNWRAP_DEPTH; depth++) {
|
||||
const inner = peelEnvelope(current);
|
||||
if (inner === null) break;
|
||||
current = inner;
|
||||
peeled = true;
|
||||
}
|
||||
|
||||
if (!peeled) return text;
|
||||
return typeof current === 'string' ? current : JSON.stringify(current, null, 2);
|
||||
}
|
||||
|
||||
/** Peel one envelope layer, or return null when this value is not a redundant wrapper. */
|
||||
function peelEnvelope(value: unknown): unknown | null {
|
||||
if (typeof value !== 'object' || value === null || Array.isArray(value)) return null;
|
||||
|
||||
const obj = value as Record<string, unknown>;
|
||||
const parts = obj['content'];
|
||||
if (!Array.isArray(parts) || parts.length === 0) return null;
|
||||
|
||||
// Only text parts can be collapsed; images and resources must survive intact.
|
||||
const texts: string[] = [];
|
||||
for (const part of parts) {
|
||||
if (typeof part !== 'object' || part === null) return null;
|
||||
const p = part as Record<string, unknown>;
|
||||
if (p['type'] !== 'text' || typeof p['text'] !== 'string') return null;
|
||||
texts.push(p['text']);
|
||||
}
|
||||
|
||||
const innerText = texts.join('\n');
|
||||
let inner: unknown;
|
||||
try {
|
||||
inner = JSON.parse(innerText);
|
||||
} catch {
|
||||
// Inner payload is plain text. Collapsing to it is still lossless when the
|
||||
// envelope adds nothing but a duplicate structuredContent.
|
||||
inner = innerText;
|
||||
}
|
||||
|
||||
// structuredContent must not carry anything the inner payload lacks.
|
||||
if ('structuredContent' in obj && !deepEqual(obj['structuredContent'], inner)) return null;
|
||||
|
||||
// Every other sibling key must already be present, and equal, inside.
|
||||
for (const [key, v] of Object.entries(obj)) {
|
||||
if (key === 'content' || key === 'structuredContent') continue;
|
||||
if (typeof inner !== 'object' || inner === null || Array.isArray(inner)) return null;
|
||||
if (!deepEqual((inner as Record<string, unknown>)[key], v)) return null;
|
||||
}
|
||||
|
||||
return inner;
|
||||
}
|
||||
|
||||
/** Structural equality for JSON-shaped values. */
|
||||
function deepEqual(a: unknown, b: unknown): boolean {
|
||||
if (a === b) return true;
|
||||
if (typeof a !== typeof b) return false;
|
||||
if (typeof a !== 'object' || a === null || b === null) return false;
|
||||
|
||||
if (Array.isArray(a) || Array.isArray(b)) {
|
||||
if (!Array.isArray(a) || !Array.isArray(b) || a.length !== b.length) return false;
|
||||
return a.every((item, i) => deepEqual(item, b[i]));
|
||||
}
|
||||
|
||||
const ao = a as Record<string, unknown>;
|
||||
const bo = b as Record<string, unknown>;
|
||||
const aKeys = Object.keys(ao);
|
||||
if (aKeys.length !== Object.keys(bo).length) return false;
|
||||
return aKeys.every((k) => k in bo && deepEqual(ao[k], bo[k]));
|
||||
}
|
||||
|
||||
/** Extract text content from a tool result response. */
|
||||
function extractTextContent(response: JsonRpcResponse): string | null {
|
||||
if (!response.result || typeof response.result !== 'object') return null;
|
||||
@@ -145,6 +334,39 @@ function handleSectionDrillDown(
|
||||
};
|
||||
}
|
||||
|
||||
/** Re-show the table of contents for a cached result (no _section given). */
|
||||
function handleSectionListing(
|
||||
request: JsonRpcRequest,
|
||||
resultId: string,
|
||||
ctx: PluginSessionContext,
|
||||
): JsonRpcResponse {
|
||||
const sections = getSections(ctx, resultId);
|
||||
if (!sections) {
|
||||
return {
|
||||
jsonrpc: '2.0',
|
||||
id: request.id,
|
||||
result: {
|
||||
content: [{
|
||||
type: 'text',
|
||||
text: 'Cached result not found (expired or invalid _resultId). Please re-call the tool without _resultId/_section to get a fresh result.',
|
||||
}],
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
const toc = sections.map((s) => `[${s.id}] ${s.title}`).join('\n');
|
||||
return {
|
||||
jsonrpc: '2.0',
|
||||
id: request.id,
|
||||
result: {
|
||||
content: [{
|
||||
type: 'text',
|
||||
text: `${sections.length} sections:\n${toc}\n\nAdd _section="<id>" alongside _resultId="${resultId}" to read one.`,
|
||||
}],
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
/** Find a section by ID, searching recursively through children. */
|
||||
function findSection(sections: Section[], id: string): Section | null {
|
||||
for (const section of sections) {
|
||||
|
||||
@@ -10,6 +10,7 @@
|
||||
* overlap on hooks, no conflicts arise.
|
||||
*/
|
||||
import type { ProxyModelPlugin } from '../plugin.js';
|
||||
import type { ToolDefinition } from '../types.js';
|
||||
import { createGatePlugin, type GatePluginConfig } from './gate.js';
|
||||
import { createContentPipelinePlugin } from './content-pipeline.js';
|
||||
|
||||
@@ -59,8 +60,15 @@ export function createDefaultPlugin(config: DefaultPluginConfig = {}): ProxyMode
|
||||
if (gate.onInitialize) {
|
||||
plugin.onInitialize = gate.onInitialize.bind(gate);
|
||||
}
|
||||
if (gate.onToolsList) {
|
||||
plugin.onToolsList = gate.onToolsList.bind(gate);
|
||||
// Tools list: gate first (it decides which tools are visible at all), then
|
||||
// content-pipeline (it annotates whatever survived with drill-down params).
|
||||
if (gate.onToolsList || pipeline.onToolsList) {
|
||||
plugin.onToolsList = async (tools, ctx): Promise<ToolDefinition[]> => {
|
||||
let acc = tools;
|
||||
if (gate.onToolsList) acc = await gate.onToolsList(acc, ctx);
|
||||
if (pipeline.onToolsList) acc = await pipeline.onToolsList(acc, ctx);
|
||||
return acc;
|
||||
};
|
||||
}
|
||||
if (pipeline.onToolCallAfter) {
|
||||
plugin.onToolCallAfter = pipeline.onToolCallAfter.bind(pipeline);
|
||||
|
||||
@@ -37,7 +37,9 @@ const handler: StageHandler = async (content, ctx) => {
|
||||
).join('\n');
|
||||
|
||||
return {
|
||||
content: `Content split into ${sections.length} pages (${content.length} total chars):\n${toc}\n\nUse section parameter to read a specific page.`,
|
||||
// No navigation hint here — the caller (content-pipeline / router) appends
|
||||
// the authoritative _resultId/_section instruction once sections are stored.
|
||||
content: `Content split into ${sections.length} pages (${content.length} total chars):\n${toc}`,
|
||||
sections,
|
||||
};
|
||||
};
|
||||
|
||||
@@ -65,7 +65,7 @@ const handler: StageHandler = async (content, ctx) => {
|
||||
}).join('\n');
|
||||
|
||||
return {
|
||||
content: `${sections.length} sections (${contentType}):\n${toc}\n\nUse section parameter to read a specific section.`,
|
||||
content: `${sections.length} sections (${contentType}):\n${toc}`,
|
||||
sections,
|
||||
};
|
||||
};
|
||||
|
||||
@@ -39,7 +39,7 @@ const handler: StageHandler = async (content, ctx) => {
|
||||
|
||||
const summary = await cachedSummarize(ctx, ctx.originalContent, maxTokens);
|
||||
return {
|
||||
content: summary + '\n\nUse section parameter with id "full" to read the complete content.',
|
||||
content: summary + '\n\nSection "full" holds the complete content.',
|
||||
sections: [{ id: 'full', title: 'Full Content', content: ctx.originalContent }],
|
||||
};
|
||||
}
|
||||
@@ -56,7 +56,7 @@ const handler: StageHandler = async (content, ctx) => {
|
||||
}).join('\n');
|
||||
|
||||
return {
|
||||
content: `${tree.length} sections:\n${toc}\n\nUse section parameter to read details.`,
|
||||
content: `${tree.length} sections:\n${toc}`,
|
||||
sections: tree,
|
||||
};
|
||||
};
|
||||
|
||||
@@ -1191,6 +1191,6 @@ function injectSectionsIntoPromptResponse(
|
||||
if (now - entry.createdAt > SECTION_TTL_MS) store.delete(key);
|
||||
}
|
||||
|
||||
const text = `${tocContent}\n\n_resultId: ${resultId} — use _resultId and _section parameters to drill into a section.`;
|
||||
const text = `${tocContent}\n\nTo read one, request this prompt again with _resultId="${resultId}" and _section="<id>". Both parameters are required together.`;
|
||||
return replacePromptText(response, text);
|
||||
}
|
||||
|
||||
339
src/mcplocal/tests/plugin-content-pipeline-drilldown.test.ts
Normal file
339
src/mcplocal/tests/plugin-content-pipeline-drilldown.test.ts
Normal file
@@ -0,0 +1,339 @@
|
||||
/**
|
||||
* content-pipeline drill-down contract tests.
|
||||
*
|
||||
* Regression cover for the defect that made UniFi tools unusable from
|
||||
* non-Claude MCP clients: a large tool result was replaced with a stub that
|
||||
* told the caller to pass _resultId/_section, but those parameters were never
|
||||
* advertised on the tool's inputSchema. Upstreams that declare
|
||||
* `additionalProperties: false` (unifi-network, my-grafana) therefore made the
|
||||
* drill-down call invalid for any client that validates against the schema,
|
||||
* and the stub's own wording ("Use section parameter") pointed at a parameter
|
||||
* that does not exist — sending it re-paginated and minted a fresh _resultId,
|
||||
* an unbounded loop.
|
||||
*
|
||||
* Driven through a real McpRouter with the default plugin (gate +
|
||||
* content-pipeline), mirroring project-mcp-endpoint wiring.
|
||||
*/
|
||||
import { describe, it, expect, vi } from 'vitest';
|
||||
import { McpRouter } from '../src/router.js';
|
||||
import type { UpstreamConnection, JsonRpcRequest, JsonRpcResponse } from '../src/types.js';
|
||||
import type { McpdClient } from '../src/http/mcpd-client.js';
|
||||
import { createDefaultPlugin } from '../src/proxymodel/plugins/default.js';
|
||||
import { LLMProviderAdapter } from '../src/proxymodel/llm-adapter.js';
|
||||
import { MemoryCache } from '../src/proxymodel/cache.js';
|
||||
|
||||
/** Mirrors unifi-network: strict schema, and a payload big enough to paginate. */
|
||||
const STRICT_SCHEMA = {
|
||||
type: 'object',
|
||||
properties: {
|
||||
targetId: { type: 'string' },
|
||||
site: { type: 'string' },
|
||||
},
|
||||
additionalProperties: false,
|
||||
$schema: 'http://json-schema.org/draft-07/schema#',
|
||||
};
|
||||
|
||||
const BIG_PAYLOAD = 'x'.repeat(20_000);
|
||||
|
||||
/**
|
||||
* The shape unifi-network actually returns: the inner MCP result is wrapped in
|
||||
* the server's own content/structuredContent pair, so the payload arrives
|
||||
* twice — once escaped inside `content`, once parsed in `structuredContent`.
|
||||
*/
|
||||
function wrapLikeUnifi(tool: string, inner: unknown): string {
|
||||
const innerEnvelope = { tool, targetId: 'home', result: inner };
|
||||
return JSON.stringify(
|
||||
{
|
||||
tool,
|
||||
targetId: 'home',
|
||||
content: [{ type: 'text', text: JSON.stringify(innerEnvelope, null, 2) }],
|
||||
structuredContent: innerEnvelope,
|
||||
},
|
||||
null,
|
||||
2,
|
||||
);
|
||||
}
|
||||
|
||||
/** Inner payload stays under the 8000-char page size; wrapped, it does not. */
|
||||
const DEVICE_ROWS = Array.from({ length: 30 }, (_, i) => ({
|
||||
mac: `0c:ea:14:38:af:${i.toString(16).padStart(2, '0')}`,
|
||||
name: `Switch ${i} — office floor plan position ${i}`,
|
||||
model: 'USPM16',
|
||||
ip: `192.168.1.${i + 10}`,
|
||||
version: '7.4.1.16850',
|
||||
}));
|
||||
|
||||
const PAYLOADS: Record<string, string> = {
|
||||
get_devices: BIG_PAYLOAD,
|
||||
get_clients: wrapLikeUnifi('get_clients', { data: DEVICE_ROWS }),
|
||||
get_alarms: wrapLikeUnifi('get_alarms', { data: [] }),
|
||||
};
|
||||
|
||||
interface Upstream {
|
||||
conn: UpstreamConnection;
|
||||
calls: Array<Record<string, unknown>>;
|
||||
}
|
||||
|
||||
function mockUpstream(name: string, payloads: Record<string, string> = PAYLOADS): Upstream {
|
||||
const calls: Array<Record<string, unknown>> = [];
|
||||
const conn = {
|
||||
name,
|
||||
isAlive: vi.fn(() => true),
|
||||
close: vi.fn(async () => {}),
|
||||
onNotification: vi.fn(),
|
||||
send: vi.fn(async (req: JsonRpcRequest): Promise<JsonRpcResponse> => {
|
||||
if (req.method === 'tools/list') {
|
||||
return {
|
||||
jsonrpc: '2.0',
|
||||
id: req.id,
|
||||
result: {
|
||||
tools: Object.keys(payloads).map((n) => ({
|
||||
name: n,
|
||||
description: `Retrieve ${n}`,
|
||||
inputSchema: STRICT_SCHEMA,
|
||||
})),
|
||||
},
|
||||
};
|
||||
}
|
||||
if (req.method === 'tools/call') {
|
||||
const params = (req.params ?? {}) as Record<string, unknown>;
|
||||
calls.push((params['arguments'] as Record<string, unknown>) ?? {});
|
||||
const tool = String(params['name'] ?? '').split('/').pop() ?? '';
|
||||
return { jsonrpc: '2.0', id: req.id, result: { content: [{ type: 'text', text: payloads[tool] ?? '' }] } };
|
||||
}
|
||||
if (req.method === 'resources/list') return { jsonrpc: '2.0', id: req.id, result: { resources: [] } };
|
||||
if (req.method === 'prompts/list') return { jsonrpc: '2.0', id: req.id, result: { prompts: [] } };
|
||||
return { jsonrpc: '2.0', id: req.id, error: { code: -32601, message: 'Not found' } };
|
||||
}),
|
||||
} as unknown as UpstreamConnection;
|
||||
return { conn, calls };
|
||||
}
|
||||
|
||||
function mockMcpdClient(): McpdClient {
|
||||
return {
|
||||
get: vi.fn(async () => []),
|
||||
post: vi.fn(async () => ({})),
|
||||
put: vi.fn(async () => ({})),
|
||||
delete: vi.fn(async () => {}),
|
||||
forward: vi.fn(async () => ({ status: 200, body: {} })),
|
||||
withHeaders: vi.fn(function (this: McpdClient) { return this; }),
|
||||
} as unknown as McpdClient;
|
||||
}
|
||||
|
||||
function setup(opts: { gated?: boolean; payloads?: Record<string, string> } = {}) {
|
||||
const router = new McpRouter();
|
||||
router.setPromptConfig(mockMcpdClient(), 'test-project');
|
||||
router.setPlugin(createDefaultPlugin({ gated: opts.gated ?? false, providerRegistry: null }));
|
||||
router.setProxyModel(
|
||||
'default',
|
||||
{ complete: async () => '', available: () => false } as unknown as LLMProviderAdapter,
|
||||
new MemoryCache(),
|
||||
);
|
||||
const upstream = mockUpstream('unifi-network', opts.payloads ?? PAYLOADS);
|
||||
router.addUpstream(upstream.conn);
|
||||
return { router, upstream };
|
||||
}
|
||||
|
||||
async function listTools(router: McpRouter, sessionId = 's1') {
|
||||
await router.route({ jsonrpc: '2.0', id: 1, method: 'initialize' }, { sessionId });
|
||||
const res = await router.route({ jsonrpc: '2.0', id: 2, method: 'tools/list' }, { sessionId });
|
||||
return (res.result as { tools: Array<{ name: string; inputSchema?: unknown }> }).tools;
|
||||
}
|
||||
|
||||
function textOf(res: JsonRpcResponse): string {
|
||||
return (res.result as { content: Array<{ text: string }> }).content[0]!.text;
|
||||
}
|
||||
|
||||
async function callNamed(
|
||||
router: McpRouter,
|
||||
tool: string,
|
||||
args: Record<string, unknown>,
|
||||
id: number,
|
||||
sessionId = 's1',
|
||||
): Promise<JsonRpcResponse> {
|
||||
return router.route(
|
||||
{ jsonrpc: '2.0', id, method: 'tools/call', params: { name: `unifi-network/${tool}`, arguments: args } },
|
||||
{ sessionId },
|
||||
);
|
||||
}
|
||||
|
||||
async function callTool(
|
||||
router: McpRouter,
|
||||
args: Record<string, unknown>,
|
||||
id: number,
|
||||
sessionId = 's1',
|
||||
): Promise<JsonRpcResponse> {
|
||||
return router.route(
|
||||
{ jsonrpc: '2.0', id, method: 'tools/call', params: { name: 'unifi-network/get_devices', arguments: args } },
|
||||
{ sessionId },
|
||||
);
|
||||
}
|
||||
|
||||
describe('content-pipeline drill-down contract', () => {
|
||||
it('advertises _resultId/_section on a strict-schema upstream tool', async () => {
|
||||
const { router } = setup();
|
||||
const tool = (await listTools(router)).find((t) => t.name.endsWith('get_devices'));
|
||||
|
||||
expect(tool).toBeDefined();
|
||||
const schema = tool!.inputSchema as Record<string, unknown>;
|
||||
const props = schema['properties'] as Record<string, unknown>;
|
||||
|
||||
expect(props['_resultId']).toMatchObject({ type: 'string' });
|
||||
expect(props['_section']).toMatchObject({ type: 'string' });
|
||||
// The upstream's own params survive untouched.
|
||||
expect(props['targetId']).toMatchObject({ type: 'string' });
|
||||
// Declaring the params in `properties` is what makes them legal; the
|
||||
// upstream's strictness is preserved rather than loosened.
|
||||
expect(schema['additionalProperties']).toBe(false);
|
||||
});
|
||||
|
||||
it('leaves gate tools alone (they never reach the pipeline)', async () => {
|
||||
const { router } = setup({ gated: true });
|
||||
const tools = await listTools(router);
|
||||
|
||||
const beginSession = tools.find((t) => t.name === 'begin_session');
|
||||
expect(beginSession).toBeDefined();
|
||||
const props = (beginSession!.inputSchema as Record<string, unknown>)['properties'] as Record<string, unknown>;
|
||||
expect(props['_resultId']).toBeUndefined();
|
||||
expect(props['_section']).toBeUndefined();
|
||||
});
|
||||
|
||||
it('stubs a large result with an instruction naming the real parameters', async () => {
|
||||
const { router } = setup();
|
||||
await listTools(router);
|
||||
|
||||
const text = textOf(await callTool(router, {}, 3));
|
||||
|
||||
expect(text).toContain('_resultId=');
|
||||
expect(text).toContain('_section=');
|
||||
// The old wording pointed at a parameter that does not exist.
|
||||
expect(text).not.toContain('Use section parameter');
|
||||
});
|
||||
|
||||
it('serves the page from cache without a second upstream call', async () => {
|
||||
const { router, upstream } = setup();
|
||||
await listTools(router);
|
||||
|
||||
const stub = textOf(await callTool(router, {}, 3));
|
||||
const resultId = /_resultId="([^"]+)"/.exec(stub)?.[1];
|
||||
expect(resultId).toBeTruthy();
|
||||
|
||||
const callsAfterFirst = upstream.calls.length;
|
||||
const page = textOf(await callTool(router, { _resultId: resultId!, _section: 'page-1' }, 4));
|
||||
|
||||
expect(page).toContain('x'.repeat(100));
|
||||
// Drill-down is answered locally, so the strict upstream never sees the
|
||||
// params it would reject.
|
||||
expect(upstream.calls.length).toBe(callsAfterFirst);
|
||||
});
|
||||
|
||||
it('re-shows the table of contents when _resultId arrives without _section', async () => {
|
||||
const { router, upstream } = setup();
|
||||
await listTools(router);
|
||||
|
||||
const stub = textOf(await callTool(router, {}, 3));
|
||||
const resultId = /_resultId="([^"]+)"/.exec(stub)?.[1];
|
||||
const callsAfterFirst = upstream.calls.length;
|
||||
|
||||
const res = textOf(await callTool(router, { _resultId: resultId! }, 4));
|
||||
|
||||
expect(res).toContain('page-1');
|
||||
expect(res).toContain('_section=');
|
||||
expect(upstream.calls.length).toBe(callsAfterFirst);
|
||||
});
|
||||
|
||||
it('reports an expired or unknown _resultId instead of forwarding it', async () => {
|
||||
const { router, upstream } = setup();
|
||||
await listTools(router);
|
||||
const callsBefore = upstream.calls.length;
|
||||
|
||||
const res = textOf(await callTool(router, { _resultId: 'pm-nope', _section: 'page-1' }, 3));
|
||||
|
||||
expect(res).toContain('Cached result not found');
|
||||
expect(upstream.calls.length).toBe(callsBefore);
|
||||
});
|
||||
});
|
||||
|
||||
describe('nested MCP envelope collapse', () => {
|
||||
it('collapses a doubly-wrapped result to its inner payload', async () => {
|
||||
const { router } = setup();
|
||||
await listTools(router);
|
||||
|
||||
const text = textOf(await callNamed(router, 'get_alarms', {}, 3));
|
||||
const parsed = JSON.parse(text) as Record<string, unknown>;
|
||||
|
||||
// The wrapper's content/structuredContent pair is gone; the payload it
|
||||
// carried twice now appears once.
|
||||
expect(parsed['content']).toBeUndefined();
|
||||
expect(parsed['structuredContent']).toBeUndefined();
|
||||
expect(parsed['tool']).toBe('get_alarms');
|
||||
expect(parsed['targetId']).toBe('home');
|
||||
expect(parsed['result']).toEqual({ data: [] });
|
||||
});
|
||||
|
||||
it('drops a formerly-paginated result below the pagination threshold', async () => {
|
||||
const { router } = setup();
|
||||
await listTools(router);
|
||||
|
||||
const wrapped = PAYLOADS['get_clients']!;
|
||||
expect(wrapped.length).toBeGreaterThan(8000); // would paginate as-is
|
||||
|
||||
const text = textOf(await callNamed(router, 'get_clients', {}, 3));
|
||||
|
||||
// Delivered whole, not as a table of contents.
|
||||
expect(text).not.toContain('Content split into');
|
||||
expect(text).not.toContain('_resultId=');
|
||||
expect(text.length).toBeLessThan(wrapped.length);
|
||||
|
||||
const parsed = JSON.parse(text) as { result: { data: unknown[] } };
|
||||
expect(parsed.result.data).toHaveLength(30);
|
||||
});
|
||||
|
||||
it('leaves a wrapper alone when structuredContent disagrees with the text', async () => {
|
||||
const divergent = JSON.stringify({
|
||||
tool: 'get_devices',
|
||||
content: [{ type: 'text', text: JSON.stringify({ tool: 'get_devices', result: { data: [1] } }) }],
|
||||
structuredContent: { tool: 'get_devices', result: { data: [1, 2, 3] } },
|
||||
});
|
||||
const { router } = setup({ payloads: { get_devices: divergent } });
|
||||
await listTools(router);
|
||||
|
||||
const text = textOf(await callTool(router, {}, 3));
|
||||
|
||||
// Collapsing here would silently drop rows, so the envelope survives.
|
||||
expect(text).toBe(divergent);
|
||||
});
|
||||
|
||||
it('leaves a wrapper alone when it carries a key the payload lacks', async () => {
|
||||
const extraKey = JSON.stringify({
|
||||
tool: 'get_devices',
|
||||
warning: 'partial results — controller unreachable',
|
||||
content: [{ type: 'text', text: JSON.stringify({ tool: 'get_devices', result: { data: [] } }) }],
|
||||
});
|
||||
const { router } = setup({ payloads: { get_devices: extraKey } });
|
||||
await listTools(router);
|
||||
|
||||
expect(textOf(await callTool(router, {}, 3))).toBe(extraKey);
|
||||
});
|
||||
|
||||
it('leaves non-text content parts intact', async () => {
|
||||
const withImage = JSON.stringify({
|
||||
tool: 'get_devices',
|
||||
content: [
|
||||
{ type: 'text', text: '{"tool":"get_devices"}' },
|
||||
{ type: 'image', data: 'iVBORw0KGgo=', mimeType: 'image/png' },
|
||||
],
|
||||
});
|
||||
const { router } = setup({ payloads: { get_devices: withImage } });
|
||||
await listTools(router);
|
||||
|
||||
expect(textOf(await callTool(router, {}, 3))).toBe(withImage);
|
||||
});
|
||||
|
||||
it('leaves plain non-JSON results untouched', async () => {
|
||||
const { router } = setup({ payloads: { get_devices: 'plain text, no envelope' } });
|
||||
await listTools(router);
|
||||
|
||||
expect(textOf(await callTool(router, {}, 3))).toBe('plain text, no envelope');
|
||||
});
|
||||
});
|
||||
@@ -261,9 +261,11 @@ describe('Prompt section drill-down', () => {
|
||||
expect(result.sections).toBeDefined();
|
||||
expect(result.sections!.length).toBeGreaterThanOrEqual(3);
|
||||
|
||||
// TOC should list sections
|
||||
// TOC should list sections. The stage must NOT name a navigation
|
||||
// parameter: the caller appends the authoritative _resultId/_section
|
||||
// instruction, and a bare `section` hint here sends models into a loop.
|
||||
expect(result.content).toContain('sections');
|
||||
expect(result.content).toContain('Use section parameter');
|
||||
expect(result.content).not.toContain('Use section parameter');
|
||||
|
||||
// Original was ~16K, TOC should be much shorter
|
||||
expect(result.content.length).toBeLessThan(largePrompt.length);
|
||||
|
||||
@@ -4,7 +4,7 @@
|
||||
* Verifies that large prompts served via prompts/get are section-split
|
||||
* and that subsequent calls with _resultId + _section return cached sections.
|
||||
*
|
||||
* Requires: mcplocal running on localhost:3200, mcpd on 10.0.0.194:3100
|
||||
* Requires: mcplocal running on localhost:3200, mcpd at https://mcpctl.ad.itaz.eu
|
||||
*/
|
||||
import { describe, it, expect, beforeAll, afterAll } from 'vitest';
|
||||
import { SmokeMcpSession, isMcplocalRunning } from './mcp-client.js';
|
||||
|
||||
@@ -6,7 +6,7 @@
|
||||
*
|
||||
* Prerequisites:
|
||||
* - mcplocal running on localhost:3200
|
||||
* - mcpd running on 10.0.0.194:3100
|
||||
* - mcpd reachable at https://mcpctl.ad.itaz.eu
|
||||
* - smoke-aws-docs server deployed (runtime: python)
|
||||
*
|
||||
* The test suite uses the fixture at fixtures/smoke-data.yaml which
|
||||
|
||||
109
src/mcplocal/tests/smoke/tool-drilldown.test.ts
Normal file
109
src/mcplocal/tests/smoke/tool-drilldown.test.ts
Normal file
@@ -0,0 +1,109 @@
|
||||
/**
|
||||
* Smoke tests: tool drill-down contract.
|
||||
*
|
||||
* A large tool result is replaced with a table of contents and re-read by
|
||||
* calling the same tool with _resultId + _section. Those two parameters must
|
||||
* appear in the tool's advertised inputSchema, because upstreams such as
|
||||
* unifi-network and my-grafana declare `additionalProperties: false` — a
|
||||
* client validating against the schema cannot send an undeclared parameter,
|
||||
* which used to make every paginated UniFi result unreadable.
|
||||
*
|
||||
* Requires: mcplocal running (localhost:3200) with a project whose servers are
|
||||
* reachable. Set SMOKE_PROJECT to target a specific project.
|
||||
*/
|
||||
import { describe, it, expect, beforeAll, afterAll } from 'vitest';
|
||||
import { SmokeMcpSession, isMcplocalRunning } from './mcp-client.js';
|
||||
|
||||
const PROJECT_NAME = process.env['SMOKE_PROJECT'] ?? 'smoke-data';
|
||||
/** HTTP-mode mcplocal authenticates every request with an McpToken. */
|
||||
const TOKEN = process.env['SMOKE_MCPTOKEN'];
|
||||
|
||||
interface Tool {
|
||||
name: string;
|
||||
inputSchema?: { type?: string; properties?: Record<string, unknown>; additionalProperties?: unknown };
|
||||
}
|
||||
|
||||
/** Tools served by the gate plugin — intercepted before the pipeline, so exempt. */
|
||||
const GATE_TOOLS = new Set(['begin_session', 'read_prompts', 'propose_prompt', 'propose_skill']);
|
||||
|
||||
describe('Smoke: tool drill-down contract', () => {
|
||||
let available = false;
|
||||
let session: SmokeMcpSession;
|
||||
let tools: Tool[] = [];
|
||||
|
||||
beforeAll(async () => {
|
||||
available = await isMcplocalRunning();
|
||||
if (!available) return;
|
||||
|
||||
session = new SmokeMcpSession(PROJECT_NAME, TOKEN);
|
||||
await session.initialize();
|
||||
await session.sendNotification('notifications/initialized');
|
||||
|
||||
// Open the gate if the project is gated, so the real catalog is visible.
|
||||
const gated = await session.send('tools/list') as { tools: Tool[] };
|
||||
if (gated.tools.some((t) => t.name === 'begin_session')) {
|
||||
await session.send('tools/call', {
|
||||
name: 'begin_session',
|
||||
arguments: { description: 'Verify the paginated tool-result drill-down contract' },
|
||||
}, 180_000);
|
||||
}
|
||||
|
||||
tools = ((await session.send('tools/list')) as { tools: Tool[] }).tools;
|
||||
}, 240_000);
|
||||
|
||||
afterAll(async () => {
|
||||
if (session) await session.close();
|
||||
});
|
||||
|
||||
it('every upstream tool advertises _resultId and _section', async () => {
|
||||
if (!available) return;
|
||||
|
||||
const upstreamTools = tools.filter((t) => !GATE_TOOLS.has(t.name));
|
||||
if (upstreamTools.length === 0) {
|
||||
console.log(` No upstream tools in project "${PROJECT_NAME}" — skipping`);
|
||||
return;
|
||||
}
|
||||
|
||||
const missing = upstreamTools.filter((t) => {
|
||||
const props = t.inputSchema?.properties;
|
||||
return props === undefined || props['_resultId'] === undefined || props['_section'] === undefined;
|
||||
});
|
||||
|
||||
if (missing.length > 0) {
|
||||
console.log(` Missing drill-down params: ${missing.map((t) => t.name).join(', ')}`);
|
||||
}
|
||||
expect(missing).toEqual([]);
|
||||
console.log(` ${upstreamTools.length} tools carry the drill-down contract`);
|
||||
});
|
||||
|
||||
it('strict upstream schemas stay strict (params are declared, not permitted)', async () => {
|
||||
if (!available) return;
|
||||
|
||||
// Declaring the params in `properties` is what makes them legal under
|
||||
// `additionalProperties: false`; loosening it instead would drop the
|
||||
// upstream's own typo protection.
|
||||
const strict = tools.filter(
|
||||
(t) => !GATE_TOOLS.has(t.name) && t.inputSchema?.additionalProperties === false,
|
||||
);
|
||||
if (strict.length === 0) {
|
||||
console.log(' No strict-schema tools in this project — skipping');
|
||||
return;
|
||||
}
|
||||
|
||||
for (const t of strict) {
|
||||
expect(t.inputSchema!.properties!['_resultId']).toBeDefined();
|
||||
expect(t.inputSchema!.properties!['_section']).toBeDefined();
|
||||
}
|
||||
console.log(` ${strict.length} strict-schema tools keep additionalProperties: false`);
|
||||
});
|
||||
|
||||
it('gate tools do not advertise drill-down params', async () => {
|
||||
if (!available) return;
|
||||
|
||||
for (const t of tools.filter((x) => GATE_TOOLS.has(x.name))) {
|
||||
const props = t.inputSchema?.properties ?? {};
|
||||
expect(props['_resultId']).toBeUndefined();
|
||||
expect(props['_section']).toBeUndefined();
|
||||
}
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user