From bb4b0b910fb4c74480ef2ed5b2e0ad535a4edf48 Mon Sep 17 00:00:00 2001 From: Michal Date: Mon, 10 Aug 2026 12:20:56 +0100 Subject: [PATCH] fix(k8s): read ApiException.code when classifying client-node errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @kubernetes/client-node v1 raises ApiException, which carries the HTTP status on `.code`. Every check in the orchestrator read only `.statusCode` — the pre-1.0 HttpError field — so `status` came back undefined and each "expected" 404/409 was rethrown instead of handled. Found in production: the first start of a server with a volume read a not-yet-existing PVC, got a 404 that should have meant "create it", and failed the instance instead. The same latent bug sat in removeContainer, where a missing pod failed the delete rather than being treated as already gone. httpStatusOf() checks .code, .statusCode and .response.statusCode, and falls back to parsing the rendered "HTTP-Code: NNN" message for wrappers that keep none of them. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_017BMXdb2qZbPSh8Q7XpTyjB --- .../services/k8s/kubernetes-orchestrator.ts | 38 +++++++++++++++---- src/mcpd/tests/k8s-orchestrator.test.ts | 25 +++++++++++- 2 files changed, 54 insertions(+), 9 deletions(-) diff --git a/src/mcpd/src/services/k8s/kubernetes-orchestrator.ts b/src/mcpd/src/services/k8s/kubernetes-orchestrator.ts index 0d6bd58..dcc2760 100644 --- a/src/mcpd/src/services/k8s/kubernetes-orchestrator.ts +++ b/src/mcpd/src/services/k8s/kubernetes-orchestrator.ts @@ -13,6 +13,32 @@ import type { K8sOfficialClientConfig } from './k8s-client-official.js'; import { generatePodSpec, generatePvcSpec, sanitizeName } from './manifest-generator.js'; import type { V1Pod, V1PersistentVolumeClaim } from '@kubernetes/client-node'; +/** + * HTTP status from a client-node error, across the shapes it actually throws. + * + * @kubernetes/client-node v1 raises `ApiException`, which carries the status on + * `.code` — not `.statusCode` as the pre-1.0 `HttpError` did. Reading only + * `.statusCode` yields undefined and turns every "expected" 404/409 into a + * rethrow, so a missing pod fails a delete and a missing PVC fails a create. + * The message-parsing fallback covers wrappers that preserve neither field. + */ +export function httpStatusOf(err: unknown): number | undefined { + const e = err as { + code?: unknown; + statusCode?: unknown; + response?: { statusCode?: unknown }; + message?: unknown; + }; + for (const candidate of [e.code, e.statusCode, e.response?.statusCode]) { + if (typeof candidate === 'number') return candidate; + } + if (typeof e.message === 'string') { + const match = /HTTP-Code:\s*(\d{3})/.exec(e.message); + if (match?.[1] !== undefined) return Number(match[1]); + } + return undefined; +} + function mapPodState(pod: V1Pod): ContainerInfo['state'] { const cs = pod.status?.containerStatuses?.[0]; if (cs) { @@ -113,8 +139,7 @@ export class KubernetesOrchestrator implements McpOrchestrator { gracePeriodSeconds: 5, }); } catch (err: unknown) { - const status = (err as { statusCode?: number }).statusCode - ?? (err as { response?: { statusCode?: number } }).response?.statusCode; + const status = httpStatusOf(err); if (status !== 404) throw err; } } @@ -323,8 +348,7 @@ export class KubernetesOrchestrator implements McpOrchestrator { body: { apiVersion: 'v1', kind: 'Namespace', metadata: { name } }, }); } catch (createErr: unknown) { - const status = (createErr as { statusCode?: number }).statusCode - ?? (createErr as { response?: { statusCode?: number } }).response?.statusCode; + const status = httpStatusOf(createErr); if (status !== 409) throw createErr; // Already exists is fine } } @@ -350,8 +374,7 @@ export class KubernetesOrchestrator implements McpOrchestrator { }); return; // Already there — reuse it, data and all. } catch (err: unknown) { - const status = (err as { statusCode?: number }).statusCode - ?? (err as { response?: { statusCode?: number } }).response?.statusCode; + const status = httpStatusOf(err); if (status !== 404) throw err; } @@ -361,8 +384,7 @@ export class KubernetesOrchestrator implements McpOrchestrator { body: generatePvcSpec(volume, this.namespace, labels) as V1PersistentVolumeClaim, }); } catch (createErr: unknown) { - const status = (createErr as { statusCode?: number }).statusCode - ?? (createErr as { response?: { statusCode?: number } }).response?.statusCode; + const status = httpStatusOf(createErr); if (status !== 409) throw createErr; // Lost a create race — fine. } } diff --git a/src/mcpd/tests/k8s-orchestrator.test.ts b/src/mcpd/tests/k8s-orchestrator.test.ts index 2f983eb..4754520 100644 --- a/src/mcpd/tests/k8s-orchestrator.test.ts +++ b/src/mcpd/tests/k8s-orchestrator.test.ts @@ -87,7 +87,7 @@ vi.mock('@kubernetes/client-node', () => { }); // Import after mock -import { KubernetesOrchestrator } from '../src/services/k8s/kubernetes-orchestrator.js'; +import { KubernetesOrchestrator, httpStatusOf } from '../src/services/k8s/kubernetes-orchestrator.js'; // eslint-disable-next-line @typescript-eslint/no-explicit-any const k8sMock = await import('@kubernetes/client-node') as any; const { setHandler, clearHandlers, mockCore } = k8sMock.__testHelpers; @@ -356,3 +356,26 @@ describe('KubernetesOrchestrator', () => { }); }); }); + +describe('httpStatusOf', () => { + it('reads .code, which is what client-node v1 ApiException actually sets', () => { + // The bug this exists for: reading only .statusCode returned undefined, so + // an expected 404 was rethrown and a first-start PVC create never happened. + expect(httpStatusOf({ code: 404 })).toBe(404); + expect(httpStatusOf({ code: 409 })).toBe(409); + }); + + it('still reads the pre-1.0 HttpError shapes', () => { + expect(httpStatusOf({ statusCode: 404 })).toBe(404); + expect(httpStatusOf({ response: { statusCode: 409 } })).toBe(409); + }); + + it('falls back to parsing the rendered message', () => { + expect(httpStatusOf(new Error('HTTP-Code: 403\nMessage: Unknown API Status Code!'))).toBe(403); + }); + + it('returns undefined when there is no status to find', () => { + expect(httpStatusOf(new Error('socket hang up'))).toBeUndefined(); + expect(httpStatusOf({ code: 'ECONNREFUSED' })).toBeUndefined(); + }); +});