fix(mcpd): recover k8s STDIO instances after in-place container restarts

A container restart (OOMKill, crash, transient npx failure) used to strand
the instance in ERROR forever while its pod sat 1/1 Running, because five
gaps lined up (#114):

- the exec/attach websocket died without ending the stdout PassThrough
  (client-node installs no onclose), so PersistentStdioClient — whose only
  death signal was stdout 'end' — kept believing it was connected and every
  request rode the 120s timeout into a dead pipe;
- the stdioClients cache is keyed by pod name, which survives a restart, so
  nothing ever evicted the corpse;
- syncStatus never re-inspected ERROR rows and never read restartCount, so
  neither the recovery nor the in-place restart was visible;
- its ERROR writes clobbered retry metadata, making the row instantly
  dueForRetry, and the retry recreated the pod under the SAME name — an
  uncaught 409 that looped ERROR against a healthy pod;
- the stuck row consumed the whole replica budget, blocking a fresh-id
  replacement.

The fix, layer by layer:
- ws 'close'/'error' now end stdout in both k8s interactive paths (and the
  docker interactive path mirrors its own one-shot handlers), funneling into
  an identity-guarded teardown in PersistentStdioClient that also listens
  for stream 'close'/'error' — a late event from a previous session cannot
  clobber a reconnected one;
- syncStatus re-inspects ERROR rows (pod running again → back to RUNNING
  with retry metadata cleared), tracks restartCount in instance metadata to
  catch restarts between polls, MERGES retry metadata instead of clobbering
  it, and evicts the cached stdio client through a setter-injected hook
  whenever the pipe is known-dead;
- createContainer adopts an alive pod on 409 instead of throwing (and only
  replaces a genuinely dead one, waiting out the deletion grace period);
- attach mode retries once through a fresh client before surfacing an error,
  so the first call after a detected death succeeds;
- the health probe evicts the cached client when failures cross the
  threshold, and shutdown finally calls closeAll().

Closes #114

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JaFvfHrQyUKCGv6o3N2Wir
This commit is contained in:
Michal
2026-08-15 23:32:23 +01:00
parent b6983f036d
commit b022f322f0
13 changed files with 754 additions and 50 deletions

View File

@@ -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');
});
});

View File

@@ -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();
});
});

View File

@@ -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);
});
});

View File

@@ -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();
});
});

View File

@@ -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);
});
});