From bd3e1134c9dbcea51557eca6a79e533b9f5f92b4 Mon Sep 17 00:00:00 2001 From: Michal Date: Thu, 20 Aug 2026 22:08:10 +0100 Subject: [PATCH 1/4] feat(secrets): survive OpenBao outages instead of cascading them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit mcpd re-read the secret backend on every use — server env resolution, LLM api keys, chat, git providers, code repos, webhooks — with no value cache, no request timeout, and a retry that only fired on HTTP 403. A few seconds of OpenBao unavailability therefore turned into minutes of degraded service: instances that restarted during the blip failed env resolution, got marked ERROR, and entered the 30s x5 then 5min backoff. Three changes, in dependency order: 1. Typed errors. `SecretNotFoundError` (definitive) vs `SecretBackendUnavailableError` (transport). The distinction has to be typed rather than string-matched — a mis-classified "not found" would resurrect deleted secrets, and a mis-classified auth failure would paper over revoked grants, which is how an upstream re-init once broke every secret write for four days (e51b924). 2. Driver resilience. Every request now carries an AbortSignal timeout (there was none, so an unreachable backend hung its caller) and retries 5xx/429/network with full-jitter backoff — 503 is what a sealed OpenBao returns and used to be an immediate hard failure. The 403 purge-and-retry stays single-shot and outside the retry budget: it is a credential refresh, not a backend-unavailable condition, and looping on it would hide a genuinely revoked grant. `healthCheck()` no longer routes through the authenticated path, so an expired role stops reporting as "OpenBao is down"; it maps OpenBao's status codes (sealed/standby/uninitialised) instead. New `authCheck()` covers the readiness half via list(), which exercises the capability we actually depend on — unlike lookup-self, which only proves the token exists. 3. CachingSecretBackendDriver. Fresh reads inside the TTL never touch the network; past it we always try the backend, and on a transport failure serve the last known-good value instead of throwing. That is what stops the ERROR storm. Deleted secrets evict and rethrow — serving those stale would resurrect a revoked credential, strictly worse than an outage — and non-transport errors rethrow untouched. plaintext is not wrapped: its read() is an identity function over the row handed in. A cold cache during an outage still fails, loudly and by design (e6cd735). Also routes BACKEND_TOKEN_DEAD / BACKEND_ROTATION_FAILED through pino rather than bare console.error. They bypassed the multistream feeding ErrorLogBuffer, so the one failure `mcpctl errors` exists to surface was the one it never showed. Tests: 20 new. The two load-bearing guards (never serve a deleted secret stale; never serve stale for a non-transport error) were confirmed to fail against deliberately broken code before being kept. The 403 purge-retry path had no coverage at all until now. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018vybEitX4FykeMatKe5Xki --- src/mcpd/src/main.ts | 16 ++ .../services/secret-backend-rotator-loop.ts | 25 ++- .../secret-backend-rotator.service.ts | 42 +++- .../src/services/secret-backend.service.ts | 28 ++- .../src/services/secret-backends/caching.ts | 198 ++++++++++++++++++ .../src/services/secret-backends/openbao.ts | 180 +++++++++++++--- .../src/services/secret-backends/types.ts | 55 ++++- src/mcpd/tests/secret-backends.test.ts | 89 ++++++++ src/mcpd/tests/secret-cache.test.ts | 198 ++++++++++++++++++ 9 files changed, 780 insertions(+), 51 deletions(-) create mode 100644 src/mcpd/src/services/secret-backends/caching.ts create mode 100644 src/mcpd/tests/secret-cache.test.ts diff --git a/src/mcpd/src/main.ts b/src/mcpd/src/main.ts index 02a92a5..4b72346 100644 --- a/src/mcpd/src/main.ts +++ b/src/mcpd/src/main.ts @@ -474,16 +474,32 @@ async function main(): Promise { }, }, secretRefResolver: secretResolverBridge, + }, { + // Cache-transition events go through pino so BACKEND_UNREACHABLE / + // BACKEND_RECOVERED land in ErrorLogBuffer and `mcpctl errors`. + log: { + warn: (obj: Record, msg: string): void => { app.log.warn(obj, msg); }, + info: (obj: Record, msg: string): void => { app.log.info(obj, msg); }, + }, }); const secretService = new SecretService(secretRepo, secretBackendService); const secretMigrateService = new SecretMigrateService(secretRepo, secretBackendService); const secretBackendRotator = new SecretBackendRotator({ backends: secretBackendService, secrets: secretService, + log: { + error: (obj: Record, msg: string): void => { app.log.error(obj, msg); }, + warn: (msg: string): void => { app.log.warn(msg); }, + }, }); const secretBackendRotatorLoop = new SecretBackendRotatorLoop({ backends: secretBackendService, rotator: secretBackendRotator, + log: { + info: (msg: string): void => { app.log.info(`[rotator] ${msg}`); }, + warn: (msg: string): void => { app.log.warn(`[rotator] ${msg}`); }, + error: (obj: Record, msg: string): void => { app.log.error(obj, msg); }, + }, }); const llmAdapters = new LlmAdapterRegistry(); // LlmService takes the adapter registry so create()/update() can run an diff --git a/src/mcpd/src/services/secret-backend-rotator-loop.ts b/src/mcpd/src/services/secret-backend-rotator-loop.ts index 2fae8ce..0cbef82 100644 --- a/src/mcpd/src/services/secret-backend-rotator-loop.ts +++ b/src/mcpd/src/services/secret-backend-rotator-loop.ts @@ -26,7 +26,11 @@ export interface SecretBackendRotatorLoopDeps { /** Override in tests. */ setTimeout?: (cb: () => void, ms: number) => NodeJS.Timeout; clearTimeout?: (t: NodeJS.Timeout) => void; - log?: { info: (msg: string) => void; warn: (msg: string) => void }; + log?: { + info: (msg: string) => void; + warn: (msg: string) => void; + error: (obj: Record, msg: string) => void; + }; } const DEFAULT_INTERVAL_MS = 24 * 3600 * 1000; @@ -36,7 +40,7 @@ export class SecretBackendRotatorLoop { private readonly timers = new Map(); private readonly setT: (cb: () => void, ms: number) => NodeJS.Timeout; private readonly clearT: (t: NodeJS.Timeout) => void; - private readonly log: { info: (msg: string) => void; warn: (msg: string) => void }; + private readonly log: NonNullable; private stopped = false; constructor(private readonly deps: SecretBackendRotatorLoopDeps) { @@ -44,9 +48,11 @@ export class SecretBackendRotatorLoop { this.clearT = deps.clearTimeout ?? ((t) => global.clearTimeout(t)); this.log = deps.log ?? { // eslint-disable-next-line no-console - info: (m) => console.log(`[rotator] ${m}`), + info: (m: string): void => { console.log(`[rotator] ${m}`); }, // eslint-disable-next-line no-console - warn: (m) => console.warn(`[rotator] ${m}`), + warn: (m: string): void => { console.warn(`[rotator] ${m}`); }, + // eslint-disable-next-line no-console + error: (obj: Record, m: string): void => { console.error(JSON.stringify({ level: 'fatal', ...obj, message: m })); }, }; } @@ -70,13 +76,10 @@ export class SecretBackendRotatorLoop { this.deps.rotator.healthCheck(b.id) .then((res) => { if (!res.ok) { - // eslint-disable-next-line no-console - console.error(JSON.stringify({ - level: 'fatal', - kind: 'BACKEND_TOKEN_DEAD', - backend: b.name, - message: res.message ?? 'unknown', - })); + this.log.error( + { kind: 'BACKEND_TOKEN_DEAD', backend: b.name }, + res.message ?? 'unknown', + ); this.log.warn(`backend '${b.name}' health check failed: ${res.message ?? 'unknown'}`); } }) diff --git a/src/mcpd/src/services/secret-backend-rotator.service.ts b/src/mcpd/src/services/secret-backend-rotator.service.ts index 0a8ed11..27a8989 100644 --- a/src/mcpd/src/services/secret-backend-rotator.service.ts +++ b/src/mcpd/src/services/secret-backend-rotator.service.ts @@ -53,18 +53,37 @@ export interface TokenMeta { rotatable?: boolean; } +/** + * Structured logger. Must be a real pino-shaped logger in production: the + * `BACKEND_TOKEN_DEAD` fatals below used to go out via bare `console.error`, + * which bypasses the pino multistream feeding `ErrorLogBuffer` — so the one + * failure `mcpctl errors` exists to surface was the one it never saw. + */ +export interface RotatorLog { + error(obj: Record, msg: string): void; + warn(msg: string): void; +} + export interface SecretBackendRotatorDeps { backends: SecretBackendService; secrets: SecretService; fetch?: typeof globalThis.fetch; now?: () => Date; + log?: RotatorLog; } export class SecretBackendRotator { private readonly now: () => Date; + private readonly log: RotatorLog; constructor(private readonly deps: SecretBackendRotatorDeps) { this.now = deps.now ?? (() => new Date()); + this.log = deps.log ?? { + // eslint-disable-next-line no-console + error: (obj: Record, msg: string): void => { console.error(JSON.stringify({ level: 'fatal', ...obj, message: msg })); }, + // eslint-disable-next-line no-console + warn: (msg: string): void => { console.warn(msg); }, + }; } /** True iff this backend is a wizard-provisioned token-auth openbao with rotation enabled. */ @@ -144,15 +163,16 @@ export class SecretBackendRotator { : err; const wrappedMsg = wrapped instanceof Error ? wrapped.message : String(wrapped); await this.recordError(backendId, meta, wrappedMsg); - // Loud, structured log so the operator sees it in `kubectl logs deploy/mcpd`. - // eslint-disable-next-line no-console - console.error(JSON.stringify({ - level: 'fatal', - kind: tokenDead ? 'BACKEND_TOKEN_DEAD' : 'BACKEND_ROTATION_FAILED', - backend: backend.name, - url: cfg.url, - message: wrappedMsg, - })); + // Loud and structured, through pino so it also lands in ErrorLogBuffer + // and therefore in `mcpctl errors` — not just in `kubectl logs`. + this.log.error( + { + kind: tokenDead ? 'BACKEND_TOKEN_DEAD' : 'BACKEND_ROTATION_FAILED', + backend: backend.name, + url: cfg.url, + }, + wrappedMsg, + ); throw wrapped; } @@ -164,7 +184,7 @@ export class SecretBackendRotator { // Log but don't fail the rotation — the new token is already live. const msg = err instanceof Error ? err.message : String(err); // eslint-disable-next-line no-console - console.warn(`rotation: revoke old accessor '${oldAccessor}' on backend '${backend.name}' failed (continuing): ${msg}`); + this.log.warn(`rotation: revoke old accessor '${oldAccessor}' on backend '${backend.name}' failed (continuing): ${msg}`); } } @@ -249,7 +269,7 @@ export class SecretBackendRotator { } catch (inner) { // Don't mask the original error — just log the DB failure. // eslint-disable-next-line no-console - console.warn(`rotation: failed to persist lastRotationError (${message}): ${inner instanceof Error ? inner.message : String(inner)}`); + this.log.warn(`rotation: failed to persist lastRotationError (${message}): ${inner instanceof Error ? inner.message : String(inner)}`); } } } diff --git a/src/mcpd/src/services/secret-backend.service.ts b/src/mcpd/src/services/secret-backend.service.ts index fcf5d97..c1578a1 100644 --- a/src/mcpd/src/services/secret-backend.service.ts +++ b/src/mcpd/src/services/secret-backend.service.ts @@ -2,6 +2,7 @@ import type { SecretBackend } from '@prisma/client'; import type { ISecretBackendRepository } from '../repositories/secret-backend.repository.js'; import type { SecretBackendDriver } from './secret-backends/types.js'; import { createDriver, type DriverFactoryDeps } from './secret-backends/factory.js'; +import { CachingSecretBackendDriver, type CachingDriverOptions, type CacheStats } from './secret-backends/caching.js'; import { NotFoundError, ConflictError } from './mcp-server.service.js'; export class SecretBackendInUseError extends Error { @@ -17,6 +18,7 @@ export class SecretBackendService { constructor( private readonly repo: ISecretBackendRepository, private readonly driverDeps: DriverFactoryDeps, + private readonly cacheOpts: CachingDriverOptions = {}, ) {} async list(): Promise { @@ -87,12 +89,34 @@ export class SecretBackendService { this.driverCache.delete(id); } - /** Get the driver for a given backend id, creating + caching on first call. */ + /** + * Get the driver for a given backend id, creating + caching on first call. + * + * Remote backends are wrapped in `CachingSecretBackendDriver` so a backend + * outage degrades to "serving last known-good" instead of failing every + * caller. `plaintext` is deliberately NOT wrapped: its `read()` is an + * identity function over the DB row passed in by the caller, so a value cache + * there would serve pre-update data with nothing to invalidate it. + * + * Config changes invalidate for free — `update()` and `delete()` drop this + * map, and the value cache lives inside the driver instance, which is right: + * if `url`/`mount`/`pathPrefix` change, the cached names now mean something + * different. + */ driverFor(backend: SecretBackend): SecretBackendDriver { const cached = this.driverCache.get(backend.id); if (cached) return cached; - const driver = createDriver(backend, this.driverDeps); + const base = createDriver(backend, this.driverDeps); + const driver = backend.type === 'plaintext' + ? base + : new CachingSecretBackendDriver(base, { ...this.cacheOpts, backendName: backend.name }); this.driverCache.set(backend.id, driver); return driver; } + + /** Cache state for a backend, for the health endpoint. Never exposes values. */ + cacheStatsFor(backend: SecretBackend): CacheStats | undefined { + const driver = this.driverFor(backend); + return driver instanceof CachingSecretBackendDriver ? driver.stats() : undefined; + } } diff --git a/src/mcpd/src/services/secret-backends/caching.ts b/src/mcpd/src/services/secret-backends/caching.ts new file mode 100644 index 0000000..22b4276 --- /dev/null +++ b/src/mcpd/src/services/secret-backends/caching.ts @@ -0,0 +1,198 @@ +/** + * Caching + stale-while-error decorator for any `SecretBackendDriver`. + * + * ## Why this exists + * + * `SecretService.resolveData()` calls `driver.read()` on *every* use, and every + * consumer funnels through it: server env resolution, LLM api keys, chat, git + * providers, code repos, webhooks. With a remote backend that means one network + * round-trip per secret per call, and — worse — any OpenBao blip propagates + * straight through. An instance that restarts during a blip fails env + * resolution, gets marked ERROR, and enters a 30s×5-then-5min backoff + * (`instance.service.ts`), so a few seconds of backend unavailability turns + * into minutes of degraded service. + * + * ## Semantics + * + * - **Fresh** (age < ttlMs): served from memory, no network. + * - **Stale-while-error**: past the TTL we always try the backend first. If it + * answers, we refresh. If it fails *as a transport failure* + * (`SecretBackendUnavailableError`), we serve the last known-good value + * instead of throwing. This is the part that actually stops the ERROR storm. + * - **`SecretNotFoundError` evicts and rethrows.** Never served stale — that + * would resurrect a deliberately deleted or revoked credential, which is + * strictly worse than an outage. + * - **Any other error rethrows, without stale.** A 403 that survives a token + * refresh means our grants were revoked; papering over it with cached data is + * exactly how an upstream OpenBao re-init once went unnoticed for four days. + * - **No negative caching.** A miss must re-check; retry/backoff already lives + * in the driver. + * + * The stale window is deliberately unbounded. A cap would mean a long outage + * eventually takes mcpd down anyway, which defeats the purpose, and the + * revoked-credential case is already handled definitively by `SecretNotFound`. + * + * ## What this does NOT fix + * + * A cold cache during an outage. If mcpd restarts while the backend is + * unreachable, nothing has a last-known-good value and secret-bearing servers + * fail to start — honestly, with a loud error. That is the correct behaviour: + * booting a server with an empty credential is the failure mode that had + * gitea-mcp reporting healthy while every authed call failed. The mitigation is + * to warm this cache at boot, not to invent a value. + * + * Values live in heap in cleartext for the TTL, so the map is bounded (LRU) and + * values are never logged. + */ +import type { SecretBackendDriver, SecretData, ExternalRef } from './types.js'; +import { SecretNotFoundError, SecretBackendUnavailableError } from './types.js'; + +export interface CachingDriverLog { + warn(obj: Record, msg: string): void; + info(obj: Record, msg: string): void; +} + +export interface CachingDriverOptions { + /** How long a value is served without consulting the backend. */ + ttlMs?: number; + /** LRU bound — these are plaintext credentials held in memory. */ + maxEntries?: number; + /** Backend name, for log context only. */ + backendName?: string; + now?: () => number; + log?: CachingDriverLog; +} + +interface CacheEntry { + data: SecretData; + fetchedAt: number; + /** Set when we last served this past its TTL because the backend was down. */ + staleSince: number | undefined; +} + +export const DEFAULT_CACHE_TTL_MS = 300_000; +export const DEFAULT_CACHE_MAX_ENTRIES = 500; + +const NOOP_LOG: CachingDriverLog = { warn: () => undefined, info: () => undefined }; + +export interface CacheStats { + entries: number; + servingStale: number; + oldestStaleSince: number | undefined; +} + +export class CachingSecretBackendDriver implements SecretBackendDriver { + readonly kind: string; + + private readonly entries = new Map(); + private readonly ttlMs: number; + private readonly maxEntries: number; + private readonly backendName: string; + private readonly nowFn: () => number; + private readonly log: CachingDriverLog; + + constructor(private readonly inner: SecretBackendDriver, opts: CachingDriverOptions = {}) { + this.kind = `cached:${inner.kind}`; + this.ttlMs = opts.ttlMs ?? DEFAULT_CACHE_TTL_MS; + this.maxEntries = opts.maxEntries ?? DEFAULT_CACHE_MAX_ENTRIES; + this.backendName = opts.backendName ?? inner.kind; + this.nowFn = opts.now ?? ((): number => Date.now()); + this.log = opts.log ?? NOOP_LOG; + } + + async read(input: { name: string; externalRef: ExternalRef; data: SecretData }): Promise { + const now = this.nowFn(); + const cached = this.entries.get(input.name); + + if (cached !== undefined && now - cached.fetchedAt < this.ttlMs) { + this.touch(input.name, cached); + return cached.data; + } + + try { + const data = await this.inner.read(input); + if (cached?.staleSince !== undefined) { + // Edge-triggered: only on the transition back to healthy. + this.log.info( + { kind: 'BACKEND_RECOVERED', backend: this.backendName, secret: input.name, + staleForMs: now - cached.staleSince }, + `secret backend '${this.backendName}' recovered; '${input.name}' is live again`, + ); + } + this.store(input.name, { data, fetchedAt: now, staleSince: undefined }); + return data; + } catch (err) { + if (err instanceof SecretNotFoundError) { + // Definitive. Drop the stale copy so we can never hand it out later. + this.entries.delete(input.name); + throw err; + } + if (!(err instanceof SecretBackendUnavailableError) || cached === undefined) { + throw err; + } + if (cached.staleSince === undefined) { + cached.staleSince = now; + this.log.warn( + { kind: 'BACKEND_UNREACHABLE', backend: this.backendName, secret: input.name, + ageMs: now - cached.fetchedAt, reason: err.message }, + `secret backend '${this.backendName}' unreachable; serving cached '${input.name}'`, + ); + } + this.touch(input.name, cached); + return cached.data; + } + } + + async write(input: { name: string; data: SecretData }): Promise<{ externalRef: ExternalRef; storedData: SecretData }> { + const result = await this.inner.write(input); + // Cache what a subsequent read() would return — the values just written — + // not `storedData`, which remote drivers deliberately leave empty. + this.store(input.name, { data: input.data, fetchedAt: this.nowFn(), staleSince: undefined }); + return result; + } + + async delete(input: { name: string; externalRef: ExternalRef }): Promise { + await this.inner.delete(input); + this.entries.delete(input.name); + } + + async list(): Promise> { + return this.inner.list(); + } + + async healthCheck(): Promise<{ ok: boolean; detail?: string }> { + return this.inner.healthCheck?.() ?? { ok: true, detail: 'no probe' }; + } + + async authCheck(): Promise<{ ok: boolean; detail?: string }> { + return this.inner.authCheck?.() ?? { ok: true, detail: 'no probe' }; + } + + /** Cache state for the backend health endpoint. Never exposes values. */ + stats(): CacheStats { + let servingStale = 0; + let oldestStaleSince: number | undefined; + for (const e of this.entries.values()) { + if (e.staleSince === undefined) continue; + servingStale++; + if (oldestStaleSince === undefined || e.staleSince < oldestStaleSince) oldestStaleSince = e.staleSince; + } + return { entries: this.entries.size, servingStale, oldestStaleSince }; + } + + /** Move an entry to the MRU end of the insertion-ordered Map. */ + private touch(name: string, entry: CacheEntry): void { + this.entries.delete(name); + this.entries.set(name, entry); + } + + private store(name: string, entry: CacheEntry): void { + this.entries.delete(name); + this.entries.set(name, entry); + while (this.entries.size > this.maxEntries) { + const oldest = this.entries.keys().next(); + if (oldest.done === true) break; + this.entries.delete(oldest.value); + } + } +} diff --git a/src/mcpd/src/services/secret-backends/openbao.ts b/src/mcpd/src/services/secret-backends/openbao.ts index 93e902c..34ddedc 100644 --- a/src/mcpd/src/services/secret-backends/openbao.ts +++ b/src/mcpd/src/services/secret-backends/openbao.ts @@ -28,6 +28,7 @@ */ import { readFile } from 'node:fs/promises'; import type { SecretBackendDriver, SecretData, ExternalRef, SecretRefResolver } from './types.js'; +import { SecretNotFoundError, SecretBackendUnavailableError } from './types.js'; /** Best-effort read of a response body for error messages. Empty on parse failure. */ async function bodyText(res: Response): Promise { @@ -77,10 +78,23 @@ export interface OpenBaoDriverDeps { readServiceAccountToken?: (path: string) => Promise; /** Clock for cache TTL — overridable in tests. */ now?: () => number; + /** Per-request timeout. Without one, an unreachable OpenBao hangs every caller. */ + timeoutMs?: number; + /** Total attempts for retryable failures (network / 5xx / 429). 1 disables retry. */ + maxAttempts?: number; + /** Base for exponential backoff between retries; full jitter is applied. */ + backoffBaseMs?: number; + /** Test seam — real sleeps would make the retry tests take seconds. */ + sleep?: (ms: number) => Promise; } const SA_TOKEN_DEFAULT_PATH = '/var/run/secrets/kubernetes.io/serviceaccount/token'; const TOKEN_RENEW_GRACE_MS = 60_000; +const DEFAULT_TIMEOUT_MS = 5_000; +const DEFAULT_MAX_ATTEMPTS = 3; +const DEFAULT_BACKOFF_BASE_MS = 200; +/** Statuses worth retrying: the backend is up but cannot answer right now. */ +const RETRYABLE_STATUS = new Set([429, 500, 502, 503, 504]); export class OpenBaoDriver implements SecretBackendDriver { readonly kind = 'openbao'; @@ -98,6 +112,10 @@ export class OpenBaoDriver implements SecretBackendDriver { private readonly resolver: SecretRefResolver | undefined; private readonly readSaToken: (path: string) => Promise; private readonly nowFn: () => number; + private readonly timeoutMs: number; + private readonly maxAttempts: number; + private readonly backoffBaseMs: number; + private readonly sleep: (ms: number) => Promise; // Cached vault token + when (epoch ms) it should be considered expired and refetched. private cachedToken: string | undefined; @@ -131,13 +149,19 @@ export class OpenBaoDriver implements SecretBackendDriver { if (deps.secretRefResolver !== undefined) this.resolver = deps.secretRefResolver; this.readSaToken = deps.readServiceAccountToken ?? ((path) => readFile(path, 'utf-8').then((s) => s.trim())); this.nowFn = deps.now ?? (() => Date.now()); + this.timeoutMs = deps.timeoutMs ?? DEFAULT_TIMEOUT_MS; + this.maxAttempts = deps.maxAttempts ?? DEFAULT_MAX_ATTEMPTS; + this.backoffBaseMs = deps.backoffBaseMs ?? DEFAULT_BACKOFF_BASE_MS; + this.sleep = deps.sleep ?? ((ms: number): Promise => new Promise((r) => { setTimeout(r, ms); })); } async read(input: { name: string; externalRef: ExternalRef; data: SecretData }): Promise { const path = this.pathFor(input.name); const res = await this.request('GET', `/v1/${this.mount}/data/${path}`); if (res.status === 404) { - throw new Error(`OpenBao: secret '${input.name}' not found at ${path}`); + // Definitive answer, not a transport failure — the caching decorator + // must evict rather than serve a stale value here. + throw new SecretNotFoundError(`OpenBao: secret '${input.name}' not found at ${path}`); } if (!res.ok) throw new Error(`OpenBao read ${path}: HTTP ${res.status} ${await bodyText(res)}`); const body = await res.json() as { data?: { data?: SecretData } }; @@ -174,10 +198,50 @@ export class OpenBaoDriver implements SecretBackendDriver { })); } + /** + * LIVENESS. Deliberately unauthenticated: `sys/health` needs no token, and + * routing it through `request()` (as this used to) took a login first — so an + * expired role reported as "OpenBao is down", and every probe cost a login. + * + * OpenBao encodes its state in the status code, so map it rather than + * collapsing everything to ok/not-ok. + */ async healthCheck(): Promise<{ ok: boolean; detail?: string }> { try { - const res = await this.request('GET', '/v1/sys/health'); - return { ok: res.ok, detail: `HTTP ${res.status}` }; + const headers: Record = {}; + if (this.namespace !== undefined) headers['X-Vault-Namespace'] = this.namespace; + const res = await this.fetchImpl(`${this.url}/v1/sys/health`, { + method: 'GET', + headers, + signal: AbortSignal.timeout(this.timeoutMs), + }); + switch (res.status) { + case 200: return { ok: true, detail: 'active' }; + case 429: return { ok: true, detail: 'standby' }; + case 472: case 473: return { ok: true, detail: 'replication secondary' }; + case 501: return { ok: false, detail: 'not initialized' }; + case 503: return { ok: false, detail: 'sealed' }; + default: return { ok: res.ok, detail: `HTTP ${String(res.status)}` }; + } + } catch (err) { + return { ok: false, detail: err instanceof Error ? err.message : String(err) }; + } + } + + /** + * READINESS. Exercises the capability we actually depend on — read/list under + * `//` — using the credentials we hold. + * + * `list()` rather than `auth/token/lookup-self` on purpose: lookup-self only + * proves the token exists, not that its policy still grants anything. The + * four-day outage in e51b924 was exactly a live token whose grants had been + * dropped by an upstream re-init. The existing read policy already permits + * this call, so it needs no bao-side change. + */ + async authCheck(): Promise<{ ok: boolean; detail?: string }> { + try { + await this.list(); + return { ok: true, detail: `readable at ${this.mount}/${this.pathPrefix}` }; } catch (err) { return { ok: false, detail: err instanceof Error ? err.message : String(err) }; } @@ -206,11 +270,28 @@ export class OpenBaoDriver implements SecretBackendDriver { const loginUrl = `${this.url}/v1/auth/${this.k8sAuthMount}/login`; const headers: Record = { 'Content-Type': 'application/json' }; if (this.namespace !== undefined) headers['X-Vault-Namespace'] = this.namespace; - const res = await this.fetchImpl(loginUrl, { - method: 'POST', - headers, - body: JSON.stringify({ role: this.k8sRole, jwt }), - }); + // Bounded like every other call: a hung login is indistinguishable from a + // hung read to the caller, and this one used to have no timeout at all. + let res: Response; + try { + res = await this.fetchImpl(loginUrl, { + method: 'POST', + headers, + body: JSON.stringify({ role: this.k8sRole, jwt }), + signal: AbortSignal.timeout(this.timeoutMs), + }); + } catch (err) { + throw new SecretBackendUnavailableError( + `OpenBao kubernetes login (role=${this.k8sRole!}): ${err instanceof Error ? err.message : String(err)}`, + { cause: err }, + ); + } + if (RETRYABLE_STATUS.has(res.status)) { + throw new SecretBackendUnavailableError( + `OpenBao kubernetes login (role=${this.k8sRole!}): HTTP ${String(res.status)}`, + { lastStatus: res.status }, + ); + } if (!res.ok) { const text = await res.text().catch(() => ''); throw new Error(`OpenBao kubernetes login (role=${this.k8sRole!}): HTTP ${String(res.status)} ${text}`); @@ -229,30 +310,77 @@ export class OpenBaoDriver implements SecretBackendDriver { return clientToken; } - private async request(method: string, path: string, body?: unknown): Promise { - const token = await this.getToken(); + /** Build a fresh RequestInit — headers must not be shared across attempts. */ + private buildInit(method: string, token: string, body?: unknown): RequestInit { const headers: Record = { 'X-Vault-Token': token }; if (this.namespace !== undefined) headers['X-Vault-Namespace'] = this.namespace; if (body !== undefined) headers['Content-Type'] = 'application/json'; - - const init: RequestInit = { method, headers }; + const init: RequestInit = { method, headers, signal: AbortSignal.timeout(this.timeoutMs) }; if (body !== undefined) init.body = JSON.stringify(body); + return init; + } - const res = await this.fetchImpl(`${this.url}${path}`, init); + /** Full-jitter exponential backoff, so concurrent callers don't resonate. */ + private backoffFor(attempt: number): number { + return Math.random() * this.backoffBaseMs * Math.pow(2, attempt - 1); + } - // If the cached token expired between cache-check and request (k8s clock - // skew, server-side revocation, etc.), purge cache and retry once. - if (res.status === 403 && this.cachedToken !== undefined) { - this.cachedToken = undefined; - this.cachedTokenExpiresAt = 0; - const fresh = await this.getToken(); - const retryHeaders: Record = { 'X-Vault-Token': fresh }; - if (this.namespace !== undefined) retryHeaders['X-Vault-Namespace'] = this.namespace; - if (body !== undefined) retryHeaders['Content-Type'] = 'application/json'; - const retryInit: RequestInit = { method, headers: retryHeaders }; - if (body !== undefined) retryInit.body = JSON.stringify(body); - return this.fetchImpl(`${this.url}${path}`, retryInit); + private async request(method: string, path: string, body?: unknown): Promise { + const url = `${this.url}${path}`; + let lastStatus: number | undefined; + let lastErr: unknown; + + for (let attempt = 1; attempt <= this.maxAttempts; attempt++) { + let res: Response; + try { + const token = await this.getToken(); + res = await this.fetchImpl(url, this.buildInit(method, token, body)); + } catch (err) { + // Network failure, DNS failure, or our own AbortSignal firing. + lastErr = err; + if (attempt < this.maxAttempts) { + await this.sleep(this.backoffFor(attempt)); + continue; + } + throw new SecretBackendUnavailableError( + `OpenBao ${method} ${path}: ${err instanceof Error ? err.message : String(err)} (after ${String(attempt)} attempt(s))`, + { cause: err }, + ); + } + + // If the cached token expired between cache-check and request (k8s clock + // skew, server-side revocation, etc.), purge cache and retry once. This + // is deliberately OUTSIDE the retry budget: it is a credential refresh, + // not a backend-unavailable condition, and it must stay single-shot so a + // genuinely revoked grant fails loudly instead of looping. + if (res.status === 403 && this.cachedToken !== undefined) { + this.cachedToken = undefined; + this.cachedTokenExpiresAt = 0; + const fresh = await this.getToken(); + return this.fetchImpl(url, this.buildInit(method, fresh, body)); + } + + // The backend is up but cannot answer right now — 503 is also what a + // sealed OpenBao returns, which used to be an immediate hard failure. + if (RETRYABLE_STATUS.has(res.status) && attempt < this.maxAttempts) { + lastStatus = res.status; + await this.sleep(this.backoffFor(attempt)); + continue; + } + if (RETRYABLE_STATUS.has(res.status)) { + throw new SecretBackendUnavailableError( + `OpenBao ${method} ${path}: HTTP ${String(res.status)} after ${String(attempt)} attempt(s)`, + { lastStatus: res.status }, + ); + } + + return res; } - return res; + + /* c8 ignore next 5 -- unreachable: every loop exit above returns or throws */ + throw new SecretBackendUnavailableError( + `OpenBao ${method} ${path}: exhausted ${String(this.maxAttempts)} attempt(s)`, + lastErr !== undefined ? { cause: lastErr, ...(lastStatus !== undefined ? { lastStatus } : {}) } : (lastStatus !== undefined ? { lastStatus } : {}), + ); } } diff --git a/src/mcpd/src/services/secret-backends/types.ts b/src/mcpd/src/services/secret-backends/types.ts index bab41d5..8d91ce9 100644 --- a/src/mcpd/src/services/secret-backends/types.ts +++ b/src/mcpd/src/services/secret-backends/types.ts @@ -46,8 +46,24 @@ export interface SecretBackendDriver { /** List everything the backend knows about. Used for migration + drift detection. */ list(): Promise>; - /** Optional: health probe. Used by `mcpctl describe secretbackend`. */ + /** + * Optional LIVENESS probe: is the backend reachable at all? + * + * Must NOT require authentication — the whole point is to separate "the + * backend is down/sealed" from "our credentials stopped working". Compare + * `authCheck()`, which is the readiness half. + */ healthCheck?(): Promise<{ ok: boolean; detail?: string }>; + + /** + * Optional READINESS probe: can we actually read through this backend with + * the credentials we hold? + * + * A backend that answers `healthCheck()` but fails here is the exact shape of + * the incident where a re-initialised OpenBao left mcpd holding valid-looking + * tokens that granted nothing. Reporting one signal for both hides it. + */ + authCheck?(): Promise<{ ok: boolean; detail?: string }>; } /** Stored config for a SecretBackend row; dispatched on `type`. */ @@ -66,3 +82,40 @@ export interface BackendRow { export interface SecretRefResolver { resolve(secretName: string, key: string): Promise; } + +/** + * The backend gave a definitive answer: this secret (or key) does not exist. + * + * Callers may treat this as final. The caching decorator EVICTS on this and + * never serves a stale value for it — serving stale here would resurrect a + * deliberately deleted or revoked credential, which is strictly worse than an + * outage. + */ +export class SecretNotFoundError extends Error { + constructor(message: string, options?: { cause?: unknown }) { + super(message, options); + this.name = 'SecretNotFoundError'; + } +} + +/** + * The backend could not be reached or did not answer: DNS/TCP failure, request + * timeout, or an exhausted retry budget against 5xx/429. + * + * This is the ONLY error the caching decorator will serve a stale value for. + * The distinction has to be typed rather than string-matched: a mis-classified + * "not found" would resurrect deleted secrets, and a mis-classified auth + * failure would silently paper over a backend whose grants were revoked — the + * failure mode that let an OpenBao re-init break every secret write for four + * days (commit e51b924). + */ +export class SecretBackendUnavailableError extends Error { + /** HTTP status of the last attempt, when the failure was an HTTP response. */ + readonly lastStatus: number | undefined; + + constructor(message: string, options?: { cause?: unknown; lastStatus?: number }) { + super(message, options?.cause !== undefined ? { cause: options.cause } : undefined); + this.name = 'SecretBackendUnavailableError'; + this.lastStatus = options?.lastStatus; + } +} diff --git a/src/mcpd/tests/secret-backends.test.ts b/src/mcpd/tests/secret-backends.test.ts index 0f86075..43785a2 100644 --- a/src/mcpd/tests/secret-backends.test.ts +++ b/src/mcpd/tests/secret-backends.test.ts @@ -1,6 +1,7 @@ import { describe, it, expect, vi } from 'vitest'; import { PlaintextDriver } from '../src/services/secret-backends/plaintext.js'; import { OpenBaoDriver } from '../src/services/secret-backends/openbao.js'; +import { SecretNotFoundError, SecretBackendUnavailableError } from '../src/services/secret-backends/types.js'; describe('PlaintextDriver', () => { const driver = new PlaintextDriver({ listAllPlaintext: async () => [{ name: 'a', data: { k: 'v' } }] }); @@ -242,3 +243,91 @@ describe('OpenBaoDriver', () => { }); }); }); + +describe('OpenBaoDriver: resilience', () => { + const resolver = { resolve: vi.fn(async () => 'test-vault-token') }; + /** No real sleeping — otherwise the backoff tests take seconds. */ + const noSleep = async (): Promise => undefined; + + function driverWith(fetchFn: ReturnType, opts: Record = {}): OpenBaoDriver { + return new OpenBaoDriver( + { url: 'http://bao.example:8200', tokenSecretRef: { name: 'bao', key: 'token' } }, + { fetch: fetchFn as unknown as typeof fetch, secretRefResolver: resolver, sleep: noSleep, ...opts }, + ); + } + + it('maps a 404 read to SecretNotFoundError', async () => { + const fetchFn = vi.fn(async () => new Response('', { status: 404 })); + await expect(driverWith(fetchFn).read({ name: 'gone', externalRef: '', data: {} })) + .rejects.toThrow(SecretNotFoundError); + }); + + it('purges the token cache and retries once on 403 — outside the retry budget', async () => { + // This path existed but was never covered; it is the revocation/re-init case. + let n = 0; + const fetchFn = vi.fn(async () => { + n++; + if (n === 1) return new Response('', { status: 403 }); + return new Response(JSON.stringify({ data: { data: { token: 'ok' } } }), { status: 200 }); + }); + const d = driverWith(fetchFn); + await expect(d.read({ name: 's', externalRef: '', data: {} })).resolves.toEqual({ token: 'ok' }); + expect(fetchFn).toHaveBeenCalledTimes(2); + }); + + it('retries a 503 (sealed) and succeeds', async () => { + let n = 0; + const fetchFn = vi.fn(async () => { + n++; + if (n < 3) return new Response('', { status: 503 }); + return new Response(JSON.stringify({ data: { data: { token: 'ok' } } }), { status: 200 }); + }); + await expect(driverWith(fetchFn).read({ name: 's', externalRef: '', data: {} })) + .resolves.toEqual({ token: 'ok' }); + expect(fetchFn).toHaveBeenCalledTimes(3); + }); + + it('throws SecretBackendUnavailableError once the retry budget is exhausted', async () => { + const fetchFn = vi.fn(async () => new Response('', { status: 503 })); + await expect(driverWith(fetchFn, { maxAttempts: 3 }).read({ name: 's', externalRef: '', data: {} })) + .rejects.toThrow(SecretBackendUnavailableError); + expect(fetchFn).toHaveBeenCalledTimes(3); + }); + + it('classifies a network/abort failure as SecretBackendUnavailableError', async () => { + const fetchFn = vi.fn(async () => { throw new DOMException('timed out', 'TimeoutError'); }); + await expect(driverWith(fetchFn, { maxAttempts: 2 }).read({ name: 's', externalRef: '', data: {} })) + .rejects.toThrow(SecretBackendUnavailableError); + expect(fetchFn).toHaveBeenCalledTimes(2); + }); + + it('passes an AbortSignal on every request', async () => { + const fetchFn = vi.fn(async () => new Response(JSON.stringify({ data: { data: {} } }), { status: 200 })); + await driverWith(fetchFn, { timeoutMs: 1234 }).read({ name: 's', externalRef: '', data: {} }); + const [, init] = fetchFn.mock.calls[0] as [unknown, RequestInit]; + expect(init.signal).toBeInstanceOf(AbortSignal); + }); + + it('healthCheck is unauthenticated and maps OpenBao status codes', async () => { + const cases: Array<[number, boolean, string]> = [ + [200, true, 'active'], + [429, true, 'standby'], + [501, false, 'not initialized'], + [503, false, 'sealed'], + ]; + for (const [status, ok, detail] of cases) { + const fetchFn = vi.fn(async () => new Response('', { status })); + const result = await driverWith(fetchFn).healthCheck(); + expect(result).toEqual({ ok, detail }); + // The whole point of the split: no token is minted for a liveness probe. + const [, init] = fetchFn.mock.calls[0] as [unknown, RequestInit]; + expect((init.headers as Record)['X-Vault-Token']).toBeUndefined(); + } + }); + + it('authCheck reports false when the token can no longer list', async () => { + const fetchFn = vi.fn(async () => new Response('', { status: 403 })); + const result = await driverWith(fetchFn).authCheck(); + expect(result.ok).toBe(false); + }); +}); diff --git a/src/mcpd/tests/secret-cache.test.ts b/src/mcpd/tests/secret-cache.test.ts new file mode 100644 index 0000000..fbbf047 --- /dev/null +++ b/src/mcpd/tests/secret-cache.test.ts @@ -0,0 +1,198 @@ +import { describe, it, expect, vi } from 'vitest'; +import { + CachingSecretBackendDriver, + type CachingDriverLog, +} from '../src/services/secret-backends/caching.js'; +import { + SecretNotFoundError, + SecretBackendUnavailableError, + type SecretBackendDriver, + type SecretData, +} from '../src/services/secret-backends/types.js'; + +/** Minimal fake backing driver whose read() behaviour the tests drive. */ +function makeInner(overrides: Partial = {}): SecretBackendDriver & { + read: ReturnType; + write: ReturnType; + delete: ReturnType; +} { + return { + kind: 'fake', + read: vi.fn(async () => ({ token: 'live' } as SecretData)), + write: vi.fn(async () => ({ externalRef: 'ref', storedData: {} as SecretData })), + delete: vi.fn(async () => undefined), + list: vi.fn(async () => []), + ...overrides, + } as never; +} + +function makeLog(): CachingDriverLog & { warns: Array>; infos: Array> } { + const warns: Array> = []; + const infos: Array> = []; + return { warns, infos, warn: (o) => { warns.push(o); }, info: (o) => { infos.push(o); } }; +} + +const REQ = { name: 'gitea-creds', externalRef: 'secret/mcpctl/gitea-creds', data: {} }; + +describe('CachingSecretBackendDriver', () => { + it('serves from cache within the TTL without touching the backend', async () => { + const inner = makeInner(); + let now = 1_000; + const d = new CachingSecretBackendDriver(inner, { ttlMs: 5_000, now: () => now }); + + expect(await d.read(REQ)).toEqual({ token: 'live' }); + now += 4_999; + expect(await d.read(REQ)).toEqual({ token: 'live' }); + + expect(inner.read).toHaveBeenCalledTimes(1); + }); + + it('refetches once the TTL has elapsed', async () => { + const inner = makeInner(); + let now = 1_000; + const d = new CachingSecretBackendDriver(inner, { ttlMs: 5_000, now: () => now }); + + await d.read(REQ); + now += 5_001; + await d.read(REQ); + + expect(inner.read).toHaveBeenCalledTimes(2); + }); + + it('serves the stale value when the backend is unavailable', async () => { + const inner = makeInner(); + let now = 1_000; + const log = makeLog(); + const d = new CachingSecretBackendDriver(inner, { ttlMs: 1_000, now: () => now, log, backendName: 'bao' }); + + await d.read(REQ); + inner.read.mockRejectedValue(new SecretBackendUnavailableError('bao down')); + now += 10_000; + + // This is the whole point: no throw, so instance.service never marks ERROR. + expect(await d.read(REQ)).toEqual({ token: 'live' }); + expect(log.warns[0]?.kind).toBe('BACKEND_UNREACHABLE'); + }); + + it('logs BACKEND_UNREACHABLE only on the transition, not on every stale read', async () => { + const inner = makeInner(); + let now = 1_000; + const log = makeLog(); + const d = new CachingSecretBackendDriver(inner, { ttlMs: 1_000, now: () => now, log }); + + await d.read(REQ); + inner.read.mockRejectedValue(new SecretBackendUnavailableError('bao down')); + for (let i = 0; i < 5; i++) { now += 2_000; await d.read(REQ); } + + expect(log.warns.filter((w) => w.kind === 'BACKEND_UNREACHABLE')).toHaveLength(1); + }); + + it('logs BACKEND_RECOVERED once the backend answers again', async () => { + const inner = makeInner(); + let now = 1_000; + const log = makeLog(); + const d = new CachingSecretBackendDriver(inner, { ttlMs: 1_000, now: () => now, log }); + + await d.read(REQ); + inner.read.mockRejectedValue(new SecretBackendUnavailableError('bao down')); + now += 2_000; + await d.read(REQ); + + inner.read.mockResolvedValue({ token: 'rotated' }); + now += 2_000; + expect(await d.read(REQ)).toEqual({ token: 'rotated' }); + expect(log.infos.filter((i) => i.kind === 'BACKEND_RECOVERED')).toHaveLength(1); + }); + + it('NEVER serves stale for a deleted secret — evicts and rethrows', async () => { + // Regression guard. Serving stale here would resurrect a revoked + // credential, which is strictly worse than an outage. + const inner = makeInner(); + let now = 1_000; + const d = new CachingSecretBackendDriver(inner, { ttlMs: 1_000, now: () => now }); + + await d.read(REQ); + inner.read.mockRejectedValue(new SecretNotFoundError('gone')); + now += 2_000; + + await expect(d.read(REQ)).rejects.toThrow(SecretNotFoundError); + expect(d.stats().entries).toBe(0); + + // And the entry really is gone — a later unavailable error has nothing to serve. + inner.read.mockRejectedValue(new SecretBackendUnavailableError('bao down')); + await expect(d.read(REQ)).rejects.toThrow(SecretBackendUnavailableError); + }); + + it('does not serve stale for a non-transport error (e.g. revoked grants)', async () => { + const inner = makeInner(); + let now = 1_000; + const d = new CachingSecretBackendDriver(inner, { ttlMs: 1_000, now: () => now }); + + await d.read(REQ); + inner.read.mockRejectedValue(new Error('OpenBao read: HTTP 403 permission denied')); + now += 2_000; + + await expect(d.read(REQ)).rejects.toThrow(/403/); + }); + + it('rethrows on a cold cache even when the backend is unavailable', async () => { + const inner = makeInner({ read: vi.fn(async () => { throw new SecretBackendUnavailableError('bao down'); }) as never }); + const d = new CachingSecretBackendDriver(inner); + + await expect(d.read(REQ)).rejects.toThrow(SecretBackendUnavailableError); + }); + + it('write() refreshes the cache so a read-after-write does not lag', async () => { + const inner = makeInner(); + const d = new CachingSecretBackendDriver(inner, { ttlMs: 60_000 }); + + await d.read(REQ); + await d.write({ name: REQ.name, data: { token: 'brand-new' } }); + + expect(await d.read(REQ)).toEqual({ token: 'brand-new' }); + expect(inner.read).toHaveBeenCalledTimes(1); + }); + + it('delete() evicts', async () => { + const inner = makeInner(); + const d = new CachingSecretBackendDriver(inner, { ttlMs: 60_000 }); + + await d.read(REQ); + await d.delete({ name: REQ.name, externalRef: REQ.externalRef }); + + expect(d.stats().entries).toBe(0); + }); + + it('bounds the map with an LRU eviction', async () => { + const inner = makeInner(); + const d = new CachingSecretBackendDriver(inner, { ttlMs: 60_000, maxEntries: 2 }); + + await d.read({ ...REQ, name: 'a' }); + await d.read({ ...REQ, name: 'b' }); + await d.read({ ...REQ, name: 'a' }); // 'a' becomes most-recently-used + await d.read({ ...REQ, name: 'c' }); // evicts 'b' + + expect(d.stats().entries).toBe(2); + inner.read.mockClear(); + await d.read({ ...REQ, name: 'a' }); + expect(inner.read).not.toHaveBeenCalled(); // 'a' survived + await d.read({ ...REQ, name: 'b' }); + expect(inner.read).toHaveBeenCalledTimes(1); // 'b' was evicted + }); + + it('reports stale count and age via stats()', async () => { + const inner = makeInner(); + let now = 1_000; + const d = new CachingSecretBackendDriver(inner, { ttlMs: 1_000, now: () => now }); + + await d.read({ ...REQ, name: 'a' }); + await d.read({ ...REQ, name: 'b' }); + expect(d.stats()).toMatchObject({ entries: 2, servingStale: 0 }); + + inner.read.mockRejectedValue(new SecretBackendUnavailableError('down')); + now += 2_000; + await d.read({ ...REQ, name: 'a' }); + + expect(d.stats()).toMatchObject({ entries: 2, servingStale: 1, oldestStaleSince: 3_000 }); + }); +}); -- 2.49.1 From 0fbfc72d68f9adebbf2b8521217968468b4994ec Mon Sep 17 00:00:00 2001 From: Michal Date: Thu, 20 Aug 2026 22:14:12 +0100 Subject: [PATCH 2/4] feat(secrets): report real backend health instead of a hard-coded tick MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `mcpctl status` derived its Secrets verdict entirely from `tokenMeta.lastRotationError`. The rotator writes that field only for `auth: 'token'` backends (SecretBackendRotator.isRotatable), so a `kubernetes`-auth backend never wrote it and the line rendered a green tick unconditionally — including with OpenBao sealed, unreachable, or answering 403 to every read. The one signal we had was structurally incapable of going red for the backend we actually run. New `GET /api/v1/secretbackends/:id/health` reports two signals, kept separate on purpose: live — reachable at all? (unauthenticated sys/health) ready — can we read through it? (uses our credentials) live-but-not-ready is the exact shape of a re-initialised OpenBao handing back valid-looking tokens that grant nothing; collapsing both into one boolean is what hid that for four days. Needs no RBAC mapping — it falls through to the generic `secretbackends` resource, so a GET is `view:secretbackends`. `mcpctl status` now renders four states — reachable / degraded (serving N cached secrets) / unreachable / auth failed — with rotation error demoted to a trailing clause rather than the verdict. A failed probe renders "? unknown", never green: not knowing is not health. JSON output carries the same probe, so scripts stop being told every k8s-auth backend is fine. Also adds a boot-time cache warm. The stale-while-error cache can only absorb an outage for secrets it has already seen, so a cold mcpd during a backend outage still fails; resolving each running server's refs once at startup closes that for the common case. Best-effort and deliberately partial — if the backend is also down at boot this is a no-op and instances fail loudly, which is correct. Persisting last-known-good to Postgres or disk would just be plaintext-at-rest again. Tests: 15 new. The five status assertions were confirmed to fail against the old rotation-only logic before being kept. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018vybEitX4FykeMatKe5Xki --- src/cli/src/commands/status.ts | 130 ++++++++++++++++-- src/cli/tests/commands/status.test.ts | 57 ++++++-- src/mcpd/src/bootstrap/warm-secret-cache.ts | 67 +++++++++ src/mcpd/src/main.ts | 15 ++ src/mcpd/src/routes/secret-backend-health.ts | 76 ++++++++++ .../tests/secret-backend-health-route.test.ts | 98 +++++++++++++ src/mcpd/tests/warm-secret-cache.test.ts | 66 +++++++++ 7 files changed, 488 insertions(+), 21 deletions(-) create mode 100644 src/mcpd/src/bootstrap/warm-secret-cache.ts create mode 100644 src/mcpd/src/routes/secret-backend-health.ts create mode 100644 src/mcpd/tests/secret-backend-health-route.test.ts create mode 100644 src/mcpd/tests/warm-secret-cache.test.ts diff --git a/src/cli/src/commands/status.ts b/src/cli/src/commands/status.ts index a523d9d..c2ce0b3 100644 --- a/src/cli/src/commands/status.ts +++ b/src/cli/src/commands/status.ts @@ -50,6 +50,7 @@ interface ServerLlm { * of the last credential-rotation failure (e.g. a dead OpenBao token). */ interface SecretBackendInfo { + id: string; name: string; type: string; isDefault?: boolean; @@ -60,6 +61,22 @@ interface SecretBackendInfo { } | null; } +/** + * Live probe result from GET /api/v1/secretbackends/:id/health. + * + * `live` and `ready` are deliberately separate: a backend that is reachable but + * whose credentials no longer grant anything is the failure mode that hid an + * OpenBao re-init for four days. `null` means the probe itself failed, which we + * report as unknown rather than pretending it means healthy. + */ +interface SecretBackendHealth { + live: boolean; + liveDetail?: string; + ready: boolean; + readyDetail?: string; + cache?: { entries: number; servingStale: number; oldestStaleSince?: number | null } | null; +} + /** * Result of a live "say hi" probe against a server LLM. `ok` says we got a * 200 + non-empty content back; `say` is the trimmed first 16 chars of the @@ -99,6 +116,7 @@ export interface StatusCommandDeps { probeServerLlm: (mcpdUrl: string, name: string, token: string | null) => Promise; /** Fetch SecretBackends from mcpd to surface backend health. Null on error. */ fetchSecretBackends: (mcpdUrl: string, token: string | null) => Promise; + probeSecretBackend: (mcpdUrl: string, id: string, token: string | null) => Promise; isTTY: boolean; } @@ -275,6 +293,38 @@ function defaultFetchSecretBackends(mcpdUrl: string, token: string | null): Prom }); } +/** + * Live-probe one SecretBackend. Resolves to null on any unhappy path — same + * never-throw discipline as the other probes here, and `null` renders as + * "unknown", never as healthy. + */ +function defaultProbeSecretBackend(mcpdUrl: string, id: string, token: string | null): Promise { + return new Promise((resolve) => { + let req: http.ClientRequest; + const headers: Record = { Accept: 'application/json' }; + if (token !== null) headers['Authorization'] = `Bearer ${token}`; + try { + req = httpDriverFor(mcpdUrl).get(`${mcpdUrl}/api/v1/secretbackends/${id}/health`, { timeout: 5000, headers }, (res) => { + if (res.statusCode !== 200) { resolve(null); res.resume(); return; } + const chunks: Buffer[] = []; + res.on('data', (chunk: Buffer) => chunks.push(chunk)); + res.on('end', () => { + try { + resolve(JSON.parse(Buffer.concat(chunks).toString('utf-8')) as SecretBackendHealth); + } catch { + resolve(null); + } + }); + }); + } catch { + resolve(null); + return; + } + req.on('error', () => resolve(null)); + req.on('timeout', () => { req.destroy(); resolve(null); }); + }); +} + /** * POST a tiny "say hi" prompt to /api/v1/llms//infer and decide if * the LLM actually serves inference. Returns ok=true when the response is @@ -386,6 +436,7 @@ const defaultDeps: StatusCommandDeps = { fetchProviders: defaultFetchProviders, fetchServerLlms: defaultFetchServerLlms, fetchSecretBackends: defaultFetchSecretBackends, + probeSecretBackend: defaultProbeSecretBackend, probeServerLlm: defaultProbeServerLlm, isTTY: process.stdout.isTTY ?? false, }; @@ -448,7 +499,7 @@ function formatProviderStatus(name: string, info: ProvidersInfo, ansi: boolean): } export function createStatusCommand(deps?: Partial): Command { - const { configDeps, credentialsDeps, log, write, checkHealth, checkLlm, fetchModels, fetchProviders, fetchServerLlms, probeServerLlm, fetchSecretBackends, isTTY } = { ...defaultDeps, ...deps }; + const { configDeps, credentialsDeps, log, write, checkHealth, checkLlm, fetchModels, fetchProviders, fetchServerLlms, probeServerLlm, fetchSecretBackends, probeSecretBackend, isTTY } = { ...defaultDeps, ...deps }; return new Command('status') .description('Show mcpctl status and connectivity') @@ -482,6 +533,30 @@ export function createStatusCommand(deps?: Partial): Command }))) : null; + // Same live probe the table view uses. `healthy` is derived from the + // probe, NOT from tokenMeta.lastRotationError — that field is only ever + // written for token-auth backends, so scripts consuming it were told + // every kubernetes-auth backend was healthy unconditionally. + const secretBackendsWithHealth = secretBackends !== null + ? await Promise.all(secretBackends.map(async (b) => { + const health = await probeSecretBackend(config.mcpdUrl, b.id, token); + return { + name: b.name, + type: b.type, + healthy: health !== null && health.live && health.ready, + live: health?.live ?? null, + ready: health?.ready ?? null, + servingStale: health?.cache?.servingStale ?? 0, + error: health === null + ? 'health probe failed' + : !health.live ? (health.liveDetail ?? 'unreachable') + : !health.ready ? (health.readyDetail ?? 'auth failed') + : null, + rotationError: b.tokenMeta?.lastRotationError ?? null, + }; + })) + : null; + const llm = llmLabel ? llmStatus === 'ok' ? llmLabel : `${llmLabel} (${llmStatus})` : null; @@ -499,7 +574,7 @@ export function createStatusCommand(deps?: Partial): Command llmStatus, ...(providersInfo ? { providers: providersInfo } : {}), ...(serverLlmsWithHealth !== null ? { serverLlms: serverLlmsWithHealth } : {}), - ...(secretBackends !== null ? { secretBackends: secretBackends.map((b) => ({ name: b.name, type: b.type, healthy: !b.tokenMeta?.lastRotationError, error: b.tokenMeta?.lastRotationError ?? null })) } : {}), + ...(secretBackends !== null ? { secretBackends: secretBackendsWithHealth } : {}), }; log(opts.output === 'json' ? formatJson(status) : formatYaml(status)); @@ -530,7 +605,7 @@ export function createStatusCommand(deps?: Partial): Command if (!llmLabel) { log(`LLM: not configured (run 'mcpctl config setup')`); - await renderSecretBackendsSection(secretBackendsPromise, isTTY); + await renderSecretBackendsSection(secretBackendsPromise, isTTY, config.mcpdUrl, token); await renderServerLlmsSection(serverLlmsPromise, config.mcpdUrl, token, isTTY); return; } @@ -595,7 +670,7 @@ export function createStatusCommand(deps?: Partial): Command } } - await renderSecretBackendsSection(secretBackendsPromise, isTTY); + await renderSecretBackendsSection(secretBackendsPromise, isTTY, config.mcpdUrl, token); await renderServerLlmsSection(serverLlmsPromise, config.mcpdUrl, token, isTTY); }); @@ -609,21 +684,50 @@ export function createStatusCommand(deps?: Partial): Command async function renderSecretBackendsSection( backendsPromise: Promise, ansi: boolean, + mcpdUrl: string, + token: string | null, ): Promise { const backends = await backendsPromise; if (backends === null || backends.length === 0) return; - const parts = backends.map((b) => { - const err = b.tokenMeta?.lastRotationError; - const tag = b.isDefault ? `${b.name}*` : b.name; - if (err) { - const short = err.split('\n')[0]?.slice(0, 80) ?? 'error'; - return ansi ? `${tag} ${RED}✗ ${short}${RESET}` : `${tag} ✗ ${short}`; - } - return ansi ? `${tag} ${GREEN}✓${RESET}` : `${tag} ✓`; - }); + const healths = await Promise.all(backends.map((b) => probeSecretBackend(mcpdUrl, b.id, token))); + const parts = backends.map((b, i) => renderOneBackend(b, healths[i] ?? null, ansi)); log(`Secrets: ${parts.join(', ')}`); } + /** + * Render one backend's status line. + * + * This used to be `tokenMeta.lastRotationError ? red : green`, which was a + * hard-coded green tick for every `auth: kubernetes` backend — the rotator + * only writes that field for token-auth backends, so it was never set and + * `mcpctl status` reported OpenBao healthy even when it was unreachable. + * Rotation state is now one clause among several, not the only signal. + */ + function renderOneBackend(b: SecretBackendInfo, health: SecretBackendHealth | null, ansi: boolean): string { + const tag = b.isDefault === true ? `${b.name}*` : b.name; + const paint = (colour: string, text: string): string => (ansi ? `${colour}${text}${RESET}` : text); + const rotationErr = b.tokenMeta?.lastRotationError ?? ''; + const rotationClause = rotationErr === '' + ? '' + : ` (rotation: ${rotationErr.split('\n')[0]?.slice(0, 60) ?? 'error'})`; + + if (health === null) { + // The probe itself failed. Unknown is not healthy — say so. + return `${tag} ${paint(YELLOW, '? unknown')}${rotationClause}`; + } + if (!health.live) { + return `${tag} ${paint(RED, `✗ unreachable: ${health.liveDetail ?? 'no detail'}`)}${rotationClause}`; + } + if (!health.ready) { + return `${tag} ${paint(RED, `✗ auth failed: ${(health.readyDetail ?? 'no detail').slice(0, 60)}`)}${rotationClause}`; + } + const stale = health.cache?.servingStale ?? 0; + if (stale > 0) { + return `${tag} ${paint(YELLOW, `⚠ degraded — serving ${String(stale)} cached secret(s)`)}${rotationClause}`; + } + return `${tag} ${paint(GREEN, '✓ reachable')}${rotationClause}`; + } + /** * Print a "Server LLMs:" section listing mcpd-managed Llm rows by tier * with a per-LLM "say hi" liveness probe. Distinct from the mcplocal-side diff --git a/src/cli/tests/commands/status.test.ts b/src/cli/tests/commands/status.test.ts index d627b55..b6b5ee9 100644 --- a/src/cli/tests/commands/status.test.ts +++ b/src/cli/tests/commands/status.test.ts @@ -30,6 +30,7 @@ function baseDeps(overrides?: Partial): Partial null, probeServerLlm: async () => ({ ok: true, ms: 12, say: 'hi' }), fetchSecretBackends: async () => null, + probeSecretBackend: async () => ({ live: true, ready: true, cache: { entries: 0, servingStale: 0 } }), isTTY: false, ...overrides, }; @@ -46,33 +47,73 @@ afterEach(() => { }); describe('status command', () => { + const BAO = { id: 'b1', name: 'bao', type: 'openbao', isDefault: true, tokenMeta: { lastRotationError: null } }; + it('shows a healthy secret backend in the Secrets line', async () => { const cmd = createStatusCommand(baseDeps({ - fetchSecretBackends: async () => [ - { name: 'bao', type: 'openbao', isDefault: true, tokenMeta: { lastRotationError: null } }, - { name: 'default', type: 'plaintext' }, - ], + fetchSecretBackends: async () => [BAO, { id: 'b2', name: 'default', type: 'plaintext' }], })); await cmd.parseAsync([], { from: 'user' }); const out = output.join('\n'); expect(out).toContain('Secrets:'); - expect(out).toContain('bao* ✓'); - expect(out).toContain('default ✓'); + expect(out).toContain('bao* ✓ reachable'); + expect(out).toContain('default ✓ reachable'); }); it('flags a dead secret-backend token in the Secrets line', async () => { const cmd = createStatusCommand(baseDeps({ fetchSecretBackends: async () => [ - { name: 'bao', type: 'openbao', isDefault: true, tokenMeta: { lastRotationError: 'BACKEND_TOKEN_DEAD: rejected the stored token\nmore detail' } }, + { ...BAO, tokenMeta: { lastRotationError: 'BACKEND_TOKEN_DEAD: rejected the stored token\nmore detail' } }, ], })); await cmd.parseAsync([], { from: 'user' }); const out = output.join('\n'); - expect(out).toContain('bao* ✗'); expect(out).toContain('BACKEND_TOKEN_DEAD'); expect(out).not.toContain('more detail'); // only first line, truncated }); + it('reports an unreachable backend even when rotation never errored', async () => { + // THE bug: a kubernetes-auth backend never writes tokenMeta.lastRotationError, + // so this line used to render a green tick with OpenBao completely down. + const cmd = createStatusCommand(baseDeps({ + fetchSecretBackends: async () => [BAO], + probeSecretBackend: async () => ({ live: false, liveDetail: 'sealed', ready: false }), + })); + await cmd.parseAsync([], { from: 'user' }); + const out = output.join('\n'); + expect(out).toContain('bao* ✗ unreachable: sealed'); + expect(out).not.toContain('✓'); + }); + + it('distinguishes reachable-but-unusable from unreachable', async () => { + const cmd = createStatusCommand(baseDeps({ + fetchSecretBackends: async () => [BAO], + probeSecretBackend: async () => ({ live: true, ready: false, readyDetail: 'HTTP 403 permission denied' }), + })); + await cmd.parseAsync([], { from: 'user' }); + expect(output.join('\n')).toContain('bao* ✗ auth failed: HTTP 403 permission denied'); + }); + + it('reports degraded while serving cached secrets', async () => { + const cmd = createStatusCommand(baseDeps({ + fetchSecretBackends: async () => [BAO], + probeSecretBackend: async () => ({ live: true, ready: true, cache: { entries: 9, servingStale: 7 } }), + })); + await cmd.parseAsync([], { from: 'user' }); + expect(output.join('\n')).toContain('bao* ⚠ degraded — serving 7 cached secret(s)'); + }); + + it('reports unknown — never healthy — when the probe itself fails', async () => { + const cmd = createStatusCommand(baseDeps({ + fetchSecretBackends: async () => [BAO], + probeSecretBackend: async () => null, + })); + await cmd.parseAsync([], { from: 'user' }); + const out = output.join('\n'); + expect(out).toContain('bao* ? unknown'); + expect(out).not.toContain('✓'); + }); + it('omits the Secrets line when mcpd returns no backends', async () => { const cmd = createStatusCommand(baseDeps({ fetchSecretBackends: async () => null })); await cmd.parseAsync([], { from: 'user' }); diff --git a/src/mcpd/src/bootstrap/warm-secret-cache.ts b/src/mcpd/src/bootstrap/warm-secret-cache.ts new file mode 100644 index 0000000..665f984 --- /dev/null +++ b/src/mcpd/src/bootstrap/warm-secret-cache.ts @@ -0,0 +1,67 @@ +/** + * One-shot: resolve every secret that a running server depends on, so the + * value cache holds a last-known-good copy before anything needs it. + * + * The caching driver absorbs a backend outage by serving the last value it saw + * — but only for secrets it has actually seen. Without this, a cold mcpd (fresh + * deploy, pod reschedule, crash-restart) has an empty cache, and if the backend + * is unreachable at that moment every secret-bearing server fails to start. + * + * This is the honest mitigation, and it is deliberately partial: if the backend + * is ALSO down at boot, this changes nothing and instances fail loudly, which + * is correct. The alternatives — persisting last-known-good to Postgres or to + * disk — are just "plaintext secrets at rest" wearing a hat, which is the thing + * we are trying to move away from. + * + * Best-effort by construction: a failure here must never block startup, and the + * warm is per-secret so one bad reference doesn't abandon the rest. + */ +import type { PrismaClient } from '@prisma/client'; +import type { SecretService } from '../services/secret.service.js'; +import type { ServerEnvEntry } from '../validation/mcp-server.schema.js'; + +export interface WarmLog { + info: (msg: string) => void; + warn: (msg: string) => void; +} + +export async function warmSecretCache( + prisma: PrismaClient, + secrets: SecretService, + log: WarmLog, +): Promise<{ warmed: number; failed: number }> { + const servers = await prisma.mcpServer.findMany({ + where: { replicas: { gt: 0 } }, + select: { name: true, env: true }, + }); + + // Distinct (secret, key) pairs — several servers commonly share one secret, + // and there is no point paying for the same read more than once. + const refs = new Map(); + for (const server of servers) { + for (const entry of (server.env ?? []) as ServerEnvEntry[]) { + const ref = entry.valueFrom?.secretRef; + if (ref === undefined) continue; + refs.set(`${ref.name}/${ref.key}`, { name: ref.name, key: ref.key }); + } + } + if (refs.size === 0) return { warmed: 0, failed: 0 }; + + let warmed = 0; + let failed = 0; + for (const ref of refs.values()) { + try { + // Value deliberately discarded — we only want it in the cache. + await secrets.resolve(ref.name, ref.key); + warmed++; + } catch { + // Expected when the backend is down, or when a server references a + // secret that no longer exists. Neither should block startup, and both + // surface loudly at instance-start time anyway. + failed++; + } + } + + log.info(`secret cache warm: ${String(warmed)} resolved, ${String(failed)} unavailable`); + return { warmed, failed }; +} diff --git a/src/mcpd/src/main.ts b/src/mcpd/src/main.ts index 4b72346..16964bd 100644 --- a/src/mcpd/src/main.ts +++ b/src/mcpd/src/main.ts @@ -25,11 +25,13 @@ import { SecretBackendService } from './services/secret-backend.service.js'; import { SecretMigrateService } from './services/secret-migrate.service.js'; import { bootstrapSecretBackends } from './bootstrap/secret-backends.js'; import { backfillSecretKeyNames } from './bootstrap/secret-key-names.js'; +import { warmSecretCache } from './bootstrap/warm-secret-cache.js'; import { registerSecretBackendRoutes } from './routes/secret-backends.js'; import { registerSecretMigrateRoutes } from './routes/secret-migrate.js'; import { SecretBackendRotator } from './services/secret-backend-rotator.service.js'; import { SecretBackendRotatorLoop } from './services/secret-backend-rotator-loop.js'; import { registerSecretBackendRotateRoutes } from './routes/secret-backend-rotate.js'; +import { registerSecretBackendHealthRoutes } from './routes/secret-backend-health.js'; import { LlmRepository } from './repositories/llm.repository.js'; import { LlmService } from './services/llm.service.js'; import { InferenceTaskRepository } from './repositories/inference-task.repository.js'; @@ -689,6 +691,7 @@ async function main(): Promise { registerSecretRoutes(app, secretService); registerSecretBackendRoutes(app, secretBackendService); registerSecretBackendRotateRoutes(app, secretBackendRotator); + registerSecretBackendHealthRoutes(app, secretBackendService); registerSecretMigrateRoutes(app, secretMigrateService); registerLlmRoutes(app, llmService); registerAgentRoutes(app, agentService); @@ -976,6 +979,18 @@ async function main(): Promise { app.log.error({ err }, 'secret keyNames backfill failed'); }); + // One-shot: pre-populate the secret value cache so a later backend outage is + // absorbed rather than cascading into instance ERROR loops. Best-effort — if + // the backend is already down at boot this is a no-op and instances fail + // honestly. See bootstrap/warm-secret-cache.ts. + warmSecretCache( + prisma, + secretService, + { info: (m: string): void => { app.log.info(m); }, warn: (m: string): void => { app.log.warn(m); } }, + ).catch((err: unknown) => { + app.log.warn({ err }, 'secret cache warm failed (non-fatal)'); + }); + // Graceful shutdown setupGracefulShutdown(app, { disconnectDb: async () => { diff --git a/src/mcpd/src/routes/secret-backend-health.ts b/src/mcpd/src/routes/secret-backend-health.ts new file mode 100644 index 0000000..8a0e457 --- /dev/null +++ b/src/mcpd/src/routes/secret-backend-health.ts @@ -0,0 +1,76 @@ +/** + * GET /api/v1/secretbackends/:id/health — a live probe of a secret backend. + * + * Exists because the only health signal we had was `tokenMeta.lastRotationError`, + * and the rotator writes that field ONLY for `auth: 'token'` backends + * (`SecretBackendRotator.isRotatable()`). A `kubernetes`-auth backend therefore + * never wrote it and rendered a hard-coded green tick in `mcpctl status` — even + * with OpenBao completely unreachable. + * + * Two signals, deliberately separate, mirroring the liveness/readiness split + * that instance health probes already use: + * + * live — is the backend reachable at all? (unauthenticated) + * ready — can we actually read through it? (uses our credentials) + * + * A backend that is `live` but not `ready` is the exact shape of a re-initialised + * OpenBao that left us holding valid-looking tokens granting nothing. Collapsing + * the two into one boolean is what hid that for four days. + * + * RBAC: no special mapping needed — `mapUrlToPermission` falls through to the + * generic `secretbackends` resource, so a GET requires `view:secretbackends`. + */ +import type { FastifyInstance } from 'fastify'; +import type { SecretBackendService } from '../services/secret-backend.service.js'; +import { NotFoundError } from '../services/mcp-server.service.js'; + +interface TokenMetaShape { + lastRotationAt?: string; + lastRotationError?: string | null; + rotatable?: boolean; +} + +export function registerSecretBackendHealthRoutes( + app: FastifyInstance, + backends: SecretBackendService, +): void { + app.get<{ Params: { id: string } }>('/api/v1/secretbackends/:id/health', async (request, reply) => { + try { + const backend = await backends.getById(request.params.id); + const driver = backends.driverFor(backend); + + const live = await driver.healthCheck?.() ?? { ok: true, detail: 'no probe' }; + // Only probe readiness if the backend answered at all — otherwise the + // auth check just re-reports the same outage with a confusing message. + const ready = live.ok + ? await driver.authCheck?.() ?? { ok: true, detail: 'no probe' } + : { ok: false, detail: 'not probed (backend unreachable)' }; + + const meta = (backend.tokenMeta ?? {}) as TokenMetaShape; + const cache = backends.cacheStatsFor(backend); + + return { + backend: backend.name, + type: backend.type, + live: live.ok, + liveDetail: live.detail, + ready: ready.ok, + readyDetail: ready.detail, + // Present only for cached (remote) backends; plaintext has no cache. + cache: cache ?? null, + rotation: { + rotatable: meta.rotatable ?? false, + lastRotationAt: meta.lastRotationAt ?? null, + lastRotationError: meta.lastRotationError ?? null, + }, + }; + } catch (err) { + if (err instanceof NotFoundError) { + reply.code(404); + return { error: err.message }; + } + reply.code(502); + return { error: err instanceof Error ? err.message : String(err) }; + } + }); +} diff --git a/src/mcpd/tests/secret-backend-health-route.test.ts b/src/mcpd/tests/secret-backend-health-route.test.ts new file mode 100644 index 0000000..809b3f2 --- /dev/null +++ b/src/mcpd/tests/secret-backend-health-route.test.ts @@ -0,0 +1,98 @@ +import { describe, it, expect, vi, afterEach } from 'vitest'; +import Fastify from 'fastify'; +import type { FastifyInstance } from 'fastify'; +import type { SecretBackend } from '@prisma/client'; +import { registerSecretBackendHealthRoutes } from '../src/routes/secret-backend-health.js'; +import { SecretBackendService } from '../src/services/secret-backend.service.js'; +import type { ISecretBackendRepository } from '../src/repositories/secret-backend.repository.js'; +import type { SecretBackendDriver } from '../src/services/secret-backends/types.js'; + +let app: FastifyInstance; +afterEach(async () => { await app?.close(); }); + +function backendRow(overrides: Partial = {}): SecretBackend { + return { + id: 'b1', name: 'bao-k8s', type: 'openbao', + config: { url: 'http://bao.example:8200', auth: 'kubernetes', role: 'mcpctl' }, + isDefault: true, description: '', version: 1, + createdAt: new Date(), updatedAt: new Date(), + ...overrides, + } as SecretBackend; +} + +/** + * Build the route over a service whose driver is stubbed. We override + * `driverFor` rather than the factory so the test drives the two probes + * directly — the point here is the route's reporting, not driver internals. + */ +async function buildApp( + probes: Pick, + row: SecretBackend = backendRow(), +): Promise { + const repo = { + findById: vi.fn(async (id: string) => (id === row.id ? row : null)), + } as unknown as ISecretBackendRepository; + const svc = new SecretBackendService(repo, { + plaintext: { listAllPlaintext: async () => [] }, + secretRefResolver: { resolve: async () => 'tok' }, + }); + vi.spyOn(svc, 'driverFor').mockReturnValue({ kind: 'openbao', ...probes } as SecretBackendDriver); + vi.spyOn(svc, 'cacheStatsFor').mockReturnValue({ entries: 3, servingStale: 0, oldestStaleSince: undefined }); + + const a = Fastify(); + registerSecretBackendHealthRoutes(a, svc); + await a.ready(); + return a; +} + +describe('GET /api/v1/secretbackends/:id/health', () => { + it('reports live+ready when the backend is fully working', async () => { + app = await buildApp({ + healthCheck: async () => ({ ok: true, detail: 'active' }), + authCheck: async () => ({ ok: true, detail: 'readable at secret/mcpctl' }), + }); + const res = await app.inject({ method: 'GET', url: '/api/v1/secretbackends/b1/health' }); + expect(res.statusCode).toBe(200); + expect(res.json()).toMatchObject({ backend: 'bao-k8s', live: true, ready: true }); + }); + + it('reports NOT live when OpenBao is sealed — regardless of rotation state', async () => { + // The bug this endpoint exists for: a kubernetes-auth backend never writes + // tokenMeta.lastRotationError, so the old status line stayed green here. + app = await buildApp({ + healthCheck: async () => ({ ok: false, detail: 'sealed' }), + authCheck: async () => ({ ok: true, detail: 'should not be consulted' }), + }); + const body = (await app.inject({ method: 'GET', url: '/api/v1/secretbackends/b1/health' })).json(); + expect(body.live).toBe(false); + expect(body.liveDetail).toBe('sealed'); + expect(body.ready).toBe(false); + expect(body.readyDetail).toMatch(/not probed/); + expect(body.rotation.lastRotationError).toBeNull(); + }); + + it('distinguishes reachable-but-unusable (revoked grants) from unreachable', async () => { + app = await buildApp({ + healthCheck: async () => ({ ok: true, detail: 'active' }), + authCheck: async () => ({ ok: false, detail: 'OpenBao list: HTTP 403 permission denied' }), + }); + const body = (await app.inject({ method: 'GET', url: '/api/v1/secretbackends/b1/health' })).json(); + expect(body).toMatchObject({ live: true, ready: false }); + expect(body.readyDetail).toMatch(/403/); + }); + + it('surfaces cache state so degraded serving is visible', async () => { + app = await buildApp({ + healthCheck: async () => ({ ok: true }), + authCheck: async () => ({ ok: true }), + }); + const body = (await app.inject({ method: 'GET', url: '/api/v1/secretbackends/b1/health' })).json(); + expect(body.cache).toMatchObject({ entries: 3, servingStale: 0 }); + }); + + it('404s for an unknown backend', async () => { + app = await buildApp({ healthCheck: async () => ({ ok: true }), authCheck: async () => ({ ok: true }) }); + const res = await app.inject({ method: 'GET', url: '/api/v1/secretbackends/nope/health' }); + expect(res.statusCode).toBe(404); + }); +}); diff --git a/src/mcpd/tests/warm-secret-cache.test.ts b/src/mcpd/tests/warm-secret-cache.test.ts new file mode 100644 index 0000000..dfc6a95 --- /dev/null +++ b/src/mcpd/tests/warm-secret-cache.test.ts @@ -0,0 +1,66 @@ +import { describe, it, expect, vi } from 'vitest'; +import { warmSecretCache } from '../src/bootstrap/warm-secret-cache.js'; +import type { PrismaClient } from '@prisma/client'; +import type { SecretService } from '../src/services/secret.service.js'; + +function prismaWith(servers: Array<{ name: string; env: unknown }>): PrismaClient { + return { mcpServer: { findMany: vi.fn(async () => servers) } } as unknown as PrismaClient; +} +const noLog = { info: (): void => undefined, warn: (): void => undefined }; + +const envRef = (name: string, secret: string, key: string): unknown => + ({ name, valueFrom: { secretRef: { name: secret, key } } }); + +describe('warmSecretCache', () => { + it('resolves every distinct secret ref exactly once', async () => { + const prisma = prismaWith([ + { name: 'gitea', env: [envRef('GITEA_ACCESS_TOKEN', 'gitea-creds', 'GITEA_ACCESS_TOKEN')] }, + // Two servers sharing one secret must not cost two reads. + { name: 'a', env: [envRef('T', 'shared', 'TOKEN')] }, + { name: 'b', env: [envRef('T', 'shared', 'TOKEN')] }, + ]); + const resolve = vi.fn(async () => 'value'); + const result = await warmSecretCache(prisma, { resolve } as unknown as SecretService, noLog); + + expect(resolve).toHaveBeenCalledTimes(2); + expect(result).toEqual({ warmed: 2, failed: 0 }); + }); + + it('ignores inline env values', async () => { + const prisma = prismaWith([{ name: 's', env: [{ name: 'PLAIN', value: 'x' }] }]); + const resolve = vi.fn(async () => 'v'); + expect(await warmSecretCache(prisma, { resolve } as unknown as SecretService, noLog)) + .toEqual({ warmed: 0, failed: 0 }); + expect(resolve).not.toHaveBeenCalled(); + }); + + it('never throws when the backend is down — startup must not block', async () => { + const prisma = prismaWith([ + { name: 'a', env: [envRef('T', 's1', 'K')] }, + { name: 'b', env: [envRef('T', 's2', 'K')] }, + ]); + const resolve = vi.fn(async () => { throw new Error('bao unreachable'); }); + await expect(warmSecretCache(prisma, { resolve } as unknown as SecretService, noLog)) + .resolves.toEqual({ warmed: 0, failed: 2 }); + }); + + it('keeps going after one bad reference', async () => { + const prisma = prismaWith([ + { name: 'a', env: [envRef('T', 'missing', 'K')] }, + { name: 'b', env: [envRef('T', 'present', 'K')] }, + ]); + const resolve = vi.fn(async (n: string) => { + if (n === 'missing') throw new Error('no such secret'); + return 'v'; + }); + expect(await warmSecretCache(prisma, { resolve } as unknown as SecretService, noLog)) + .toEqual({ warmed: 1, failed: 1 }); + }); + + it('only considers servers with replicas > 0', async () => { + const prisma = prismaWith([]); + await warmSecretCache(prisma, { resolve: vi.fn() } as unknown as SecretService, noLog); + const findMany = (prisma.mcpServer.findMany as unknown as ReturnType); + expect(findMany.mock.calls[0]?.[0]).toMatchObject({ where: { replicas: { gt: 0 } } }); + }); +}); -- 2.49.1 From fe9987cefd430ab7017e7b8e5aeba87b15e88f55 Mon Sep 17 00:00:00 2001 From: Michal Date: Thu, 20 Aug 2026 22:16:04 +0100 Subject: [PATCH 3/4] test(secrets): smoke-cover backend health honesty + cache correctness MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two of these four assertions currently FAIL against the live cluster, which is the point: mcpd there still renders `Secrets: bao-k8s* ✓` from the rotation field, and its JSON status carries no live/ready pair. Per the project rule the fix is to deploy, not to relax the assertion. The status check specifically rejects a bare "name ✓" with no qualifier — that string is the old rendering and proves the probe was never consulted. Deliberately does not take the real OpenBao down. Simulating an outage against shared infrastructure to satisfy a test would be worse than the bug it covers; the outage paths are unit-tested with an injected clock. Docs: adds a Reliability section (request hardening, the stale-while-error table, the cold-cache gap and why persisting values is not the answer, live-vs-ready) and replaces the stale "Kubernetes ServiceAccount auth is not shipped yet" note — it shipped in 5152066, and it is what the live backend has used since June. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018vybEitX4FykeMatKe5Xki --- docs/secret-backends.md | 106 +++++++++++++- .../smoke/secret-resilience.smoke.test.ts | 133 ++++++++++++++++++ 2 files changed, 237 insertions(+), 2 deletions(-) create mode 100644 src/mcplocal/tests/smoke/secret-resilience.smoke.test.ts diff --git a/docs/secret-backends.md b/docs/secret-backends.md index d53d358..78df3bf 100644 --- a/docs/secret-backends.md +++ b/docs/secret-backends.md @@ -118,8 +118,110 @@ That's the whole point of keeping plaintext around — it's the trust root: token itself. DB access is now equivalent to OpenBao token access (a single key), not equivalent to all API keys in the system. -Follow-up work (not shipped yet) replaces static token auth with Kubernetes -ServiceAccount auth so no bootstrap token is needed at all. +#### Kubernetes ServiceAccount auth (no bootstrap token) + +`auth: kubernetes` removes the chicken-and-egg entirely: mcpd exchanges its +projected ServiceAccount JWT for an OpenBao token at +`auth//role/`, so there is no static credential in the database +at all. The token is cached for its lease and re-minted lazily with a 60s grace +window. + +```yaml +kind: secretbackend +name: bao-k8s +type: openbao +isDefault: true +config: + url: https://bao.example + auth: kubernetes + role: mcpctl + authMount: kubernetes-worker0 # defaults to `kubernetes` +``` + +Note that the daily **rotator does not apply** to these backends — there is no +stored token to rotate. That has a consequence for monitoring, see below. + +## Reliability + +Remote backends are network dependencies on the critical path of nearly +everything: server env resolution, LLM api keys, chat, git providers, code +repos, webhooks. Three mechanisms keep an outage from cascading. + +### Request hardening + +Every call carries a timeout (default 5s) and retries `5xx`/`429`/network +failures with full-jitter exponential backoff (3 attempts). A **sealed** OpenBao +answers `503`, so this covers unseal windows and failovers. + +The `403` path is separate and deliberately single-shot: the driver purges its +cached token, re-authenticates and retries **once**. That is a credential +refresh, not a backend-unavailable condition — looping on it would hide a +genuinely revoked grant. + +### Value cache with stale-while-error + +Resolved values are cached per backend (default TTL 5 minutes, LRU-bounded). +Past the TTL the backend is always consulted; if it fails *as a transport +failure*, the last known-good value is served instead of throwing. + +| Failure | Behaviour | +|---|---| +| Backend unreachable / timeout / exhausted 5xx | Serve last known-good, mark degraded, log `BACKEND_UNREACHABLE` once | +| Secret deleted (404) | **Evict and throw.** Never served stale — that would resurrect a revoked credential | +| 403 after a token refresh | Throw. Revoked grants must stay loud | +| Nothing cached yet | Throw | + +The stale window is unbounded on purpose: a cap would mean a long outage +eventually takes mcpd down anyway. + +`plaintext` backends are not cached — their `read()` is an identity function +over the row the caller already supplied. + +**Cold cache is the known gap.** If mcpd restarts *while* the backend is +unreachable, nothing has a last-known-good value and secret-bearing servers fail +to start. That is deliberate: booting a server with an empty credential is worse +(gitea-mcp once ran for weeks with an empty `GITEA_ACCESS_TOKEN`, answering +`tools/list` and reporting healthy while every authenticated call failed). mcpd +mitigates it by warming the cache at boot — one read per referenced secret — so +an outage that starts *after* startup is fully absorbed. + +### Health: `live` vs `ready` + +```bash +curl $MCPD/api/v1/secretbackends//health +``` + +```json +{ "live": true, "liveDetail": "active", + "ready": false, "readyDetail": "OpenBao list: HTTP 403 permission denied", + "cache": { "entries": 9, "servingStale": 0 }, + "rotation": { "rotatable": false, "lastRotationError": null } } +``` + +- **`live`** — unauthenticated `sys/health`. Distinguishes *down* from *sealed* + from *standby*. +- **`ready`** — a real read with our credentials. + +The two are separate because `live && !ready` is a distinct, important state: a +re-initialised OpenBao hands back valid-looking tokens that grant nothing. +Collapsing them into one boolean is what let that go unnoticed for four days. + +`mcpctl status` renders the probe directly: + +``` +Secrets: bao-k8s* ✓ reachable, default ✓ reachable +Secrets: bao-k8s* ⚠ degraded — serving 7 cached secret(s) +Secrets: bao-k8s* ✗ unreachable: sealed +Secrets: bao-k8s* ✗ auth failed: HTTP 403 permission denied +Secrets: bao-k8s* ? unknown +``` + +> **Historical note.** This verdict used to come solely from +> `tokenMeta.lastRotationError`, which only the rotator writes — and the rotator +> skips `auth: kubernetes` backends. The Secrets line was therefore *incapable* +> of going red for a k8s-auth backend, and reported OpenBao healthy while it was +> unreachable. `?` (probe failed) renders yellow, never green: not knowing is not +> health. ## Migration — `mcpctl migrate secrets` diff --git a/src/mcplocal/tests/smoke/secret-resilience.smoke.test.ts b/src/mcplocal/tests/smoke/secret-resilience.smoke.test.ts new file mode 100644 index 0000000..358bf84 --- /dev/null +++ b/src/mcplocal/tests/smoke/secret-resilience.smoke.test.ts @@ -0,0 +1,133 @@ +/** + * Smoke tests: secret-backend health honesty + value caching, against live mcpd. + * + * Covers the two behaviours that unit tests cannot prove, because both are + * about what the REAL backend and the REAL CLI do together: + * + * 1. `mcpctl status` reports a probed verdict, not a hard-coded tick. The bug + * being guarded is that the verdict used to come from + * `tokenMeta.lastRotationError`, which a `kubernetes`-auth backend never + * writes — so the line was structurally incapable of going red. + * 2. The value cache does not corrupt reads, and a delete really evicts. + * + * Deliberately does NOT take the real backend down. Simulating an outage + * against shared infrastructure to satisfy a test would be worse than the bug. + * + * Target: mcpd direct (`--direct`), same skip-if-unreachable discipline as the + * other smokes here. + * + * Run with: pnpm test:smoke + */ +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import http from 'node:http'; +import https from 'node:https'; +import { execSync } from 'node:child_process'; + +const MCPD_URL = process.env.MCPD_URL ?? 'https://mcpctl.ad.itaz.eu'; +const SECRET_NAME = `smoke-cache-${Date.now().toString(36)}`; + +interface CliResult { code: number; stdout: string; stderr: string } + +function run(args: string): CliResult { + try { + return { code: 0, stdout: execSync(`mcpctl --direct ${args}`, { encoding: 'utf-8', timeout: 30_000, stdio: ['ignore', 'pipe', 'pipe'] }).trim(), stderr: '' }; + } catch (err) { + const e = err as { status?: number; stdout?: Buffer | string; stderr?: Buffer | string }; + return { + code: e.status ?? 1, + stdout: e.stdout ? String(e.stdout) : '', + stderr: e.stderr ? String(e.stderr) : '', + }; + } +} + +function healthz(url: string, timeoutMs = 5000): Promise { + return new Promise((resolve) => { + const parsed = new URL(`${url.replace(/\/$/, '')}/healthz`); + const driver = parsed.protocol === 'https:' ? https : http; + const req = driver.get( + { hostname: parsed.hostname, port: parsed.port || (parsed.protocol === 'https:' ? 443 : 80), path: parsed.pathname, timeout: timeoutMs }, + (res) => { resolve((res.statusCode ?? 500) < 500); res.resume(); }, + ); + req.on('error', () => resolve(false)); + req.on('timeout', () => { req.destroy(); resolve(false); }); + }); +} + +let mcpdUp = false; + +describe('secret resilience smoke', () => { + beforeAll(async () => { + mcpdUp = await healthz(MCPD_URL); + if (!mcpdUp) { + // eslint-disable-next-line no-console + console.warn(`\n ○ secret resilience smoke: skipped — ${MCPD_URL}/healthz unreachable. Set MCPD_URL to override.\n`); + } + }, 20_000); + + afterAll(() => { + if (!mcpdUp) return; + run(`delete secret ${SECRET_NAME}`); + }); + + it('status reports a probed backend verdict, not an unconditional tick', () => { + if (!mcpdUp) return; + const result = run('status'); + expect(result.code, result.stderr).toBe(0); + const line = result.stdout.split('\n').find((l) => l.startsWith('Secrets:')); + expect(line, 'status must include a Secrets: line').toBeDefined(); + // The verdict must be one the live probe can produce. A bare "name ✓" with + // no qualifier is the OLD rendering and means the probe was not consulted. + expect(line).toMatch(/reachable|degraded|unreachable|auth failed|unknown/); + }); + + it('reports live and ready separately per backend in JSON output', () => { + if (!mcpdUp) return; + const result = run('status -o json'); + expect(result.code, result.stderr).toBe(0); + const parsed = JSON.parse(result.stdout) as { + secretBackends?: Array<{ name: string; healthy: boolean; live: boolean | null; ready: boolean | null }>; + }; + expect(parsed.secretBackends, 'JSON status must carry secretBackends').toBeDefined(); + for (const b of parsed.secretBackends ?? []) { + // Both signals present and independent — not one boolean copied twice. + expect(b, `backend ${b.name}`).toHaveProperty('live'); + expect(b, `backend ${b.name}`).toHaveProperty('ready'); + expect(b.healthy).toBe(b.live === true && b.ready === true); + } + }); + + it('caching does not corrupt repeated reads, and delete evicts', () => { + if (!mcpdUp) return; + const created = run(`create secret ${SECRET_NAME} --data TOKEN=cache-probe-value`); + expect(created.code, created.stderr).toBe(0); + + // Two reads back-to-back: the second is a cache hit. Both must agree. + const first = run(`describe secret ${SECRET_NAME} --show-values`); + const second = run(`describe secret ${SECRET_NAME} --show-values`); + expect(first.code, first.stderr).toBe(0); + expect(second.code, second.stderr).toBe(0); + expect(first.stdout).toContain('cache-probe-value'); + expect(second.stdout).toContain('cache-probe-value'); + + // Delete must evict — a cached value surviving a delete is exactly the + // "resurrected revoked credential" failure the cache guards against. + const deleted = run(`delete secret ${SECRET_NAME}`); + expect(deleted.code, deleted.stderr).toBe(0); + const after = run(`describe secret ${SECRET_NAME} --show-values`); + expect(after.code, 'reading a deleted secret must fail, not serve cache').not.toBe(0); + }); + + it('exposes the per-backend health endpoint used by status', () => { + if (!mcpdUp) return; + const backends = run('get secretbackends -o json'); + expect(backends.code, backends.stderr).toBe(0); + const rows = JSON.parse(backends.stdout) as Array<{ id: string; name: string }>; + expect(rows.length).toBeGreaterThan(0); + // describe must surface the same probe, for every backend type — the old + // Token health block was gated on tokenMeta.rotatable and so rendered + // nothing at all for kubernetes-auth backends. + const described = run(`describe secretbackend ${rows[0]?.name ?? ''}`); + expect(described.code, described.stderr).toBe(0); + }); +}); -- 2.49.1 From 545e7745dafe627c94c9e0fb1e840153e6f812a4 Mon Sep 17 00:00:00 2001 From: Michal Date: Thu, 20 Aug 2026 22:17:24 +0100 Subject: [PATCH 4/4] test(secrets): cover the rotator loop's boot-time dead-token detection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SecretBackendRotatorLoop had zero tests, despite being the detector added in e51b924 specifically so a re-initialised OpenBao surfaces the moment mcpd boots rather than 24h later when the scheduled rotation finally fires. The class already injects setTimeout/clearTimeout and a logger, so this needed no production change. Nine cases, weighted to what actually breaks: the boot health check runs per rotatable backend; a dead token emits kind BACKEND_TOKEN_DEAD through the injected logger (the point of the earlier console.error removal — a bare console call never reaches ErrorLogBuffer, so `mcpctl errors` could not see it); a throwing health check does not abort start(); overdue backends rotate immediately and still get scheduled; the 60s floor holds across 50 adversarial-jitter draws; and stop() both clears timers and trips the `stopped` guard against rescheduling, which had never been exercised. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018vybEitX4FykeMatKe5Xki --- .../tests/secret-backend-rotator-loop.test.ts | 155 ++++++++++++++++++ 1 file changed, 155 insertions(+) create mode 100644 src/mcpd/tests/secret-backend-rotator-loop.test.ts diff --git a/src/mcpd/tests/secret-backend-rotator-loop.test.ts b/src/mcpd/tests/secret-backend-rotator-loop.test.ts new file mode 100644 index 0000000..197ff47 --- /dev/null +++ b/src/mcpd/tests/secret-backend-rotator-loop.test.ts @@ -0,0 +1,155 @@ +/** + * SecretBackendRotatorLoop had no coverage at all, despite being the boot-time + * detector added after an upstream OpenBao re-init silently broke every secret + * write for four days (e51b924). These pin the behaviours that matter when that + * recurs: the boot health check fires, it reports through the injected logger + * (so `mcpctl errors` sees it), and stop() genuinely stops. + */ +import { describe, it, expect, vi } from 'vitest'; +import type { SecretBackend } from '@prisma/client'; +import { SecretBackendRotatorLoop } from '../src/services/secret-backend-rotator-loop.js'; +import type { SecretBackendService } from '../src/services/secret-backend.service.js'; +import type { SecretBackendRotator } from '../src/services/secret-backend-rotator.service.js'; + +function backend(overrides: Partial = {}): SecretBackend { + return { + id: 'b1', name: 'bao', type: 'openbao', + config: { url: 'http://bao.example:8200', rotation: { enabled: true, tokenRole: 'r', intervalHours: 24 } }, + isDefault: true, description: '', version: 1, + createdAt: new Date(), updatedAt: new Date(), + ...overrides, + } as SecretBackend; +} + +interface Harness { + loop: SecretBackendRotatorLoop; + rotator: { isRotatable: ReturnType; isOverdue: ReturnType; healthCheck: ReturnType; rotateOne: ReturnType }; + logs: { info: string[]; warn: string[]; error: Array<{ obj: Record; msg: string }> }; + timers: Array<{ cb: () => void; ms: number }>; + cleared: number; +} + +function harness(opts: { + rows?: SecretBackend[]; + rotatable?: boolean; + overdue?: boolean; + health?: { ok: boolean; message?: string } | Error; +} = {}): Harness { + const rows = opts.rows ?? [backend()]; + const logs: Harness['logs'] = { info: [], warn: [], error: [] }; + const timers: Harness['timers'] = []; + const state = { cleared: 0 }; + + const rotator = { + isRotatable: vi.fn(() => opts.rotatable ?? true), + isOverdue: vi.fn(() => opts.overdue ?? false), + healthCheck: vi.fn(async () => { + if (opts.health instanceof Error) throw opts.health; + return opts.health ?? { ok: true }; + }), + rotateOne: vi.fn(async () => ({})), + }; + + const loop = new SecretBackendRotatorLoop({ + backends: { + list: async () => rows, + getById: async (id: string) => rows.find((r) => r.id === id) ?? rows[0]!, + } as unknown as SecretBackendService, + rotator: rotator as unknown as SecretBackendRotator, + setTimeout: ((cb: () => void, ms: number) => { timers.push({ cb, ms }); return { id: timers.length } as unknown as NodeJS.Timeout; }), + clearTimeout: (() => { state.cleared++; }), + log: { + info: (m) => { logs.info.push(m); }, + warn: (m) => { logs.warn.push(m); }, + error: (obj, msg) => { logs.error.push({ obj, msg }); }, + }, + }); + + return { loop, rotator, logs, timers, get cleared() { return state.cleared; } } as Harness; +} + +/** The boot health check is fire-and-forget; let its microtasks settle. */ +const settle = async (): Promise => { await Promise.resolve(); await Promise.resolve(); await Promise.resolve(); }; + +describe('SecretBackendRotatorLoop', () => { + it('stays idle when nothing is rotatable', async () => { + const h = harness({ rotatable: false }); + await h.loop.start(); + expect(h.logs.info.join(' ')).toMatch(/no rotatable backends/); + expect(h.timers).toHaveLength(0); + expect(h.rotator.healthCheck).not.toHaveBeenCalled(); + }); + + it('runs a boot-time health check for every rotatable backend', async () => { + const h = harness({ rows: [backend(), backend({ id: 'b2', name: 'bao2' })] }); + await h.loop.start(); + await settle(); + expect(h.rotator.healthCheck).toHaveBeenCalledTimes(2); + }); + + it('emits BACKEND_TOKEN_DEAD through the logger, not console', async () => { + // The regression that made `mcpctl errors` blind to it: this used to be a + // bare console.error, which bypasses the pino stream feeding ErrorLogBuffer. + const h = harness({ health: { ok: false, message: 'token rejected' } }); + await h.loop.start(); + await settle(); + expect(h.logs.error).toHaveLength(1); + expect(h.logs.error[0]?.obj).toMatchObject({ kind: 'BACKEND_TOKEN_DEAD', backend: 'bao' }); + expect(h.logs.error[0]?.msg).toBe('token rejected'); + }); + + it('does not log a fatal when the backend is healthy', async () => { + const h = harness({ health: { ok: true } }); + await h.loop.start(); + await settle(); + expect(h.logs.error).toHaveLength(0); + }); + + it('survives a health check that throws', async () => { + const h = harness({ health: new Error('network down') }); + await expect(h.loop.start()).resolves.toBeUndefined(); + await settle(); + expect(h.logs.warn.join(' ')).toMatch(/health check threw: network down/); + }); + + it('rotates immediately when a backend is overdue, and still schedules', async () => { + const h = harness({ overdue: true }); + await h.loop.start(); + await settle(); + expect(h.rotator.rotateOne).toHaveBeenCalledWith('b1'); + expect(h.timers).toHaveLength(1); + }); + + it('does not rotate on boot when not overdue', async () => { + const h = harness({ overdue: false }); + await h.loop.start(); + await settle(); + expect(h.rotator.rotateOne).not.toHaveBeenCalled(); + expect(h.timers).toHaveLength(1); + }); + + it('never schedules sooner than the 60s floor, even with adversarial jitter', async () => { + // intervalHours tiny + default jitter would otherwise produce a negative delay. + const rows = [backend({ config: { url: 'u', rotation: { enabled: true, tokenRole: 'r', intervalHours: 0.0001 } } } as Partial)]; + for (let i = 0; i < 50; i++) { + const h = harness({ rows }); + await h.loop.start(); + expect(h.timers[0]?.ms).toBeGreaterThanOrEqual(60_000); + } + }); + + it('stop() clears timers and suppresses further scheduling', async () => { + const h = harness(); + await h.loop.start(); + expect(h.timers).toHaveLength(1); + + h.loop.stop(); + expect(h.cleared).toBeGreaterThan(0); + + // The `stopped` guard has never been exercised: a firing timer must not + // reschedule after stop(). + const before = h.timers.length; + await h.loop.rotateNow('b1').catch(() => undefined); + expect(h.timers).toHaveLength(before); + }); +}); -- 2.49.1