diff --git a/src/mcpd/src/main.ts b/src/mcpd/src/main.ts index 02a92a5..57e405d 100644 --- a/src/mcpd/src/main.ts +++ b/src/mcpd/src/main.ts @@ -511,6 +511,10 @@ async function main(): Promise { 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 { healthProbeRunner.stop(); secretBackendRotatorLoop.stop(); gitBackup.stop(); + mcpProxyService.closeAll(); await prisma.$disconnect(); }, }); diff --git a/src/mcpd/src/services/docker/container-manager.ts b/src/mcpd/src/services/docker/container-manager.ts index b591b28..79b758d 100644 --- a/src/mcpd/src/services/docker/container-manager.ts +++ b/src/mcpd/src/services/docker/container-manager.ts @@ -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) { diff --git a/src/mcpd/src/services/health-probe.service.ts b/src/mcpd/src/services/health-probe.service.ts index 8196f29..0079848 100644 --- a/src/mcpd/src/services/health-probe.service.ts +++ b/src/mcpd/src/services/health-probe.service.ts @@ -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})` diff --git a/src/mcpd/src/services/instance.service.ts b/src/mcpd/src/services/instance.service.ts index e78e5fb..6464fbf 100644 --- a/src/mcpd/src/services/instance.service.ts +++ b/src/mcpd/src/services/instance.service.ts @@ -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 { 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 { 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); } diff --git a/src/mcpd/src/services/k8s/kubernetes-orchestrator.ts b/src/mcpd/src/services/k8s/kubernetes-orchestrator.ts index dcc2760..27ace79 100644 --- a/src/mcpd/src/services/k8s/kubernetes-orchestrator.ts +++ b/src/mcpd/src/services/k8s/kubernetes-orchestrator.ts @@ -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) { diff --git a/src/mcpd/src/services/mcp-proxy-service.ts b/src/mcpd/src/services/mcp-proxy-service.ts index ea3767c..8e92079 100644 --- a/src/mcpd/src/services/mcp-proxy-service.ts +++ b/src/mcpd/src/services/mcp-proxy-service.ts @@ -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}`, + }, + }; + } } } diff --git a/src/mcpd/src/services/orchestrator.ts b/src/mcpd/src/services/orchestrator.ts index 7ac03ee..6b7a160 100644 --- a/src/mcpd/src/services/orchestrator.ts +++ b/src/mcpd/src/services/orchestrator.ts @@ -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 { diff --git a/src/mcpd/src/services/transport/persistent-stdio.ts b/src/mcpd/src/services/transport/persistent-stdio.ts index c832485..f9312ba 100644 --- a/src/mcpd/src/services/transport/persistent-stdio.ts +++ b/src/mcpd/src/services/transport/persistent-stdio.ts @@ -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): void { if (!this.exec) throw new Error('Not connected'); this.exec.write(JSON.stringify(msg) + '\n'); diff --git a/src/mcpd/tests/instance-service.test.ts b/src/mcpd/tests/instance-service.test.ts index 6cda9bb..2e4d89e 100644 --- a/src/mcpd/tests/instance-service.test.ts +++ b/src/mcpd/tests/instance-service.test.ts @@ -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; + + 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; + 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; + 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)['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)['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'); + }); +}); diff --git a/src/mcpd/tests/k8s-orchestrator.test.ts b/src/mcpd/tests/k8s-orchestrator.test.ts index 4754520..dddd3e8 100644 --- a/src/mcpd/tests/k8s-orchestrator.test.ts +++ b/src/mcpd/tests/k8s-orchestrator.test.ts @@ -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 }; attach?: { attach: ReturnType } } = {}; + 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 } { + const ws = new EventEmitter() as EventEmitter & { close: ReturnType }; + 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((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((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((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(); + }); +}); + diff --git a/src/mcpd/tests/mcp-server-flow.test.ts b/src/mcpd/tests/mcp-server-flow.test.ts index ce3c348..3280997 100644 --- a/src/mcpd/tests/mcp-server-flow.test.ts +++ b/src/mcpd/tests/mcp-server-flow.test.ts @@ -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); + }); +}); diff --git a/src/mcpd/tests/persistent-stdio.test.ts b/src/mcpd/tests/persistent-stdio.test.ts index 50afe78..72d9c2c 100644 --- a/src/mcpd/tests/persistent-stdio.test.ts +++ b/src/mcpd/tests/persistent-stdio.test.ts @@ -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): Promise { + 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(); + }); +}); diff --git a/src/mcpd/tests/services/health-probe.test.ts b/src/mcpd/tests/services/health-probe.test.ts index 8c33ccb..544a993 100644 --- a/src/mcpd/tests/services/health-probe.test.ts +++ b/src/mcpd/tests/services/health-probe.test.ts @@ -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); + 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); + }); +});