michal/mcpctl
1
0
Fork 0
Code Issues Pull Requests 7 Actions Packages Projects Releases Wiki Activity

fix(mcpd): fail-loud on env resolution + retry/backoff + readiness via proxy

Browse Source 
Three connected issues with how instances came up + got reported as
healthy when their secret backend was unreachable. The motivating
case: gitea-mcp-server starts when mcpd can't read the
gitea-creds secret from OpenBao, runs with an empty
GITEA_ACCESS_TOKEN, replies fine to tools/list (so liveness passes),
but every authed call fails with "token is required" — and
`mcpctl get instances` cheerfully reports the instance as healthy.

## What changed

### 1. Env resolution failures are now fatal for the start attempt

`src/mcpd/src/services/instance.service.ts`

The previous behaviour swallowed `resolveServerEnv` failures and let
the container start anyway with whatever env survived ("non-fatal —
container may still work if env vars are optional"). That's the bug:
the gitea container started with no token, ran for weeks, and was
reported healthy.

The catch now calls `markInstanceError(instance, "secret resolution
failed: <reason>")` and returns. Optional/missing env vars should be
modelled as `value: ""` entries on the server, not as silent
secret-resolution failures.

### 2. ERROR instances retry with backoff, not blind churn

Adds Kubernetes-style escalation: 30 s × 5 attempts, then 5 min
pauses thereafter. Retry state lives on `McpInstance.metadata` (no
schema migration) — `attemptCount`, `lastAttemptAt`, `nextRetryAt`,
`error`.

The reconciler no longer tears down ERROR instances and creates
fresh replacements (which would reset attemptCount and effectively
loop at 30 s forever). Instead:

- ERROR rows whose `nextRetryAt` is in the future are LEFT ALONE
  and counted against the replica budget — preventing tight create-
  fail-create churn while a previous attempt is in its backoff window.
- ERROR rows whose `nextRetryAt` has elapsed are retried IN-PLACE
  via a new `retryInstance` method, which preserves attemptCount on
  the same row so the schedule actually escalates.

The work has been factored into `startOne` (creates + initial attempt)
+ `attemptStart` (env + container) + `retryInstance` (re-attempt the
same row) + `markInstanceError` (write retry metadata).

### 3. STDIO readiness probe goes through mcpProxyService

`src/mcpd/src/services/health-probe.service.ts`

The legacy `probeStdio` (a `docker exec node -e '... spawn(packageName)
...'` invocation) only worked for packageName-based servers. Image-
based STDIO servers like gitea-mcp-server fell through with "No
packageName or command for STDIO server" and were reported unhealthy
for the WRONG reason — they have no packageName because they are an
image, not because anything's wrong.

New `probeReadinessViaProxy`: sends `tools/call` through the live
running container via `mcpProxyService.execute`. Same code path as
production traffic, so probe failures match real failures. Picks up:

- JSON-RPC errors (e.g. "token is required" when env is empty).
- Tool-level errors expressed as `result.isError: true`.
- Connection failures wrapped as exceptions.
- Hard timeouts via the deadline race.

After this PR, configuring `gitea` with
`healthCheck: { tool: get_me, intervalSeconds: 60 }` makes
`mcpctl get instances` report it as `unhealthy` whenever the auth
token is missing or wrong — which is honest.

The dead `probeStdio` (~120 LOC) is removed; HTTP/SSE bespoke probe
paths are kept for now (they work and the diff stays minimal).

## Tests

`src/mcpd/tests/instance-service.test.ts`:
- Replaces "cleans up ERROR instances and creates replacements" with
  "retries ERROR instances in-place when their backoff has elapsed".
- Adds "leaves ERROR instances alone while their nextRetryAt is in
  the future" and "escalates the backoff: attemptCount + nextRetryAt
  persist on retry failures".

`src/mcpd/tests/services/health-probe.test.ts`:
- Swaps STDIO probe mocks from `orchestrator.execInContainer` →
  `mcpProxyService.execute`.
- Adds "marks unhealthy when proxy returns a JSON-RPC error
  (e.g. broken-secret auth failure)" — explicitly the gitea case.
- Adds "marks unhealthy when proxy returns a tool-level error in
  result.isError" — covers servers that report tool failures as
  isError instead of as JSON-RPC errors.
- Renames "handles exec timeout" → "handles probe timeout" and
  exercises the deadline race rather than an exec rejection.

Full suite: 162 test files / 2161 tests green (+4 new).

## Manual verification step (post-deploy)

```bash
mcpctl edit server gitea
# → add healthCheck:
#     tool: get_me
#     intervalSeconds: 60
#     timeoutSeconds: 10
#     failureThreshold: 3
```

If OpenBao is still down: gitea instance enters ERROR with
attemptCount + nextRetryAt visible in `mcpctl describe instance`.
Otherwise: gitea env resolves at next start, probe passes, instance
is honestly healthy.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Michal
2026-05-07 18:55:23 +01:00
parent 56735a5290
commit e6cd73543a
4 changed files with 414 additions and 183 deletions
Download Patch File Download Diff File Expand all files Collapse all files

179
src/mcpd/src/services/instance.service.ts
View File

@@ -1,4 +1,4 @@
import type { McpInstance } from '@prisma/client';
import type { McpInstance, McpServer } from '@prisma/client';
import type { IMcpInstanceRepository, IMcpServerRepository } from '../repositories/interfaces.js';
import type { McpOrchestrator, ContainerSpec, ContainerInfo } from './orchestrator.js';
import { NotFoundError } from './mcp-server.service.js';
@@ -13,6 +13,36 @@ const RUNNER_IMAGES: Record<string, string> = {
/** Network for MCP server containers (matches docker-compose mcp-servers network). */
const MCP_SERVERS_NETWORK = process.env['MCPD_MCP_NETWORK'] ?? 'mcp-servers';
/**
* Backoff schedule for instance startup failures (env resolution, container
* creation, etc). Mirrors Kubernetes-style escalation: fast retries for
* transient hiccups, then a longer pause once it's clear something is
* persistently wrong.
*
* The retry state lives on `McpInstance.metadata` (no schema migration
* needed) and is preserved across reconcile cycles by the in-place
* `retryInstance` path so attemptCount actually accumulates.
*/
const FAST_RETRY_MS = 30_000; // first 5 attempts: 30s apart
const SLOW_RETRY_MS = 5 * 60_000; // afterwards: 5 minutes
const MAX_FAST_RETRIES = 5;
interface RetryMetadata {
error?: string;
attemptCount?: number;
lastAttemptAt?: string;
nextRetryAt?: string;
[k: string]: unknown;
}
function readRetryMeta(instance: McpInstance): RetryMetadata {
return (instance.metadata ?? {}) as RetryMetadata;
}
function nextDelayMs(attemptCount: number): number {
return attemptCount <= MAX_FAST_RETRIES ? FAST_RETRY_MS : SLOW_RETRY_MS;
}
export class InvalidStateError extends Error {
readonly statusCode = 409;
constructor(message: string) {
@@ -118,8 +148,12 @@ export class InstanceService {
* Reconcile ALL servers — the operator loop.
*
* For every server with replicas > 0, ensures the correct number of
* healthy instances exist. Cleans up ERROR instances and starts
* replacements. This is the core self-healing mechanism.
* healthy instances exist. ERROR instances are not blindly recreated:
* within their `nextRetryAt` window they're left alone (and counted
* against the replica budget so we don't churn replacements while one
* is in backoff); past their window they're retried in-place via
* `retryInstance` so attemptCount accumulates and backoff escalates
* correctly.
*/
async reconcileAll(): Promise<{ reconciled: number; errors: string[] }> {
await this.syncStatus();
@@ -128,6 +162,8 @@ export class InstanceService {
let reconciled = 0;
const errors: string[] = [];
const now = Date.now();
for (const server of servers) {
if (server.replicas <= 0) continue;
@@ -136,17 +172,38 @@ export class InstanceService {
const active = instances.filter((i) => i.status === 'RUNNING' || i.status === 'STARTING');
const errored = instances.filter((i) => i.status === 'ERROR');
// Clean up ERROR instances so they don't accumulate
// Partition ERROR instances by whether their backoff window has elapsed.
const dueForRetry: McpInstance[] = [];
const stillWaiting: McpInstance[] = [];
for (const inst of errored) {
await this.removeOne(inst);
const meta = readRetryMeta(inst);
const ts = meta.nextRetryAt ? Date.parse(meta.nextRetryAt) : 0;
if (Number.isNaN(ts) || ts <= now) {
dueForRetry.push(inst);
} else {
stillWaiting.push(inst);
}
}
// Scale up if needed
const toStart = server.replicas - active.length;
// Retry elapsed ones in-place. This preserves attemptCount across
// attempts so the 30s × 5 → 5min schedule actually escalates.
for (const inst of dueForRetry) {
await this.retryInstance(inst);
}
// Scale up only if we don't already have enough live attempts.
// Live attempts = currently-running OR -starting + still-waiting
// (in backoff) + just-retried (now STARTING via retryInstance).
// Counting waiting + retried against the budget prevents tight
// create-fail-create churn while previous attempts work through
// their backoff schedule.
const toStart = server.replicas - active.length - stillWaiting.length - dueForRetry.length;
if (toStart > 0) {
for (let i = 0; i < toStart; i++) {
await this.startOne(server.id);
}
}
if (toStart > 0 || dueForRetry.length > 0) {
reconciled++;
}
} catch (err) {
@@ -220,7 +277,12 @@ export class InstanceService {
return this.orchestrator.getContainerLogs(instance.containerId, opts);
}
/** Start a single instance for a server. */
/**
* Start a single instance for a server. Creates a fresh `STARTING` row
* and hands off to `attemptStart` for the env+container work. On
* failure, `attemptStart` marks the row `ERROR` with a backoff-aware
* `nextRetryAt`; the reconciler picks it up later via `retryInstance`.
*/
private async startOne(serverId: string): Promise<McpInstance> {
const server = await this.serverRepo.findById(serverId);
if (!server) throw new NotFoundError(`McpServer '${serverId}' not found`);
@@ -234,6 +296,49 @@ export class InstanceService {
});
}
const instance = await this.instanceRepo.create({
serverId,
status: 'STARTING',
});
return this.attemptStart(instance, server);
}
/**
* Re-attempt a previously-errored instance in place, preserving its
* `attemptCount` so the backoff schedule escalates correctly. Called
* by `reconcileAll` for ERROR instances whose `nextRetryAt` has elapsed.
*/
private async retryInstance(instance: McpInstance): Promise<McpInstance> {
const server = await this.serverRepo.findById(instance.serverId);
if (!server) {
// Server was deleted underneath us — nothing to retry against.
return this.markInstanceError(instance, 'Server no longer exists');
}
if (server.externalUrl) {
// External servers don't need a container; the URL is the contract.
return this.instanceRepo.updateStatus(instance.id, 'RUNNING', {
metadata: { external: true, url: server.externalUrl },
});
}
// Reset transient fields but keep retry counters via the metadata
// passed through `attemptStart` → `markInstanceError`.
await this.instanceRepo.updateStatus(instance.id, 'STARTING', {});
const refreshed = (await this.instanceRepo.findById(instance.id)) ?? instance;
return this.attemptStart(refreshed, server);
}
/**
* Run the env-resolution + container-creation steps for a STARTING
* instance. On any failure, mark the instance `ERROR` with structured
* retry metadata. Used by both initial start (`startOne`) and retry
* (`retryInstance`).
*/
private async attemptStart(
instance: McpInstance,
server: McpServer,
): Promise<McpInstance> {
// Determine image + command based on server config:
// 1. Explicit dockerImage → use as-is
// 2. packageName → use runtime-specific runner image (node/python/go/...)
@@ -253,11 +358,6 @@ export class InstanceService {
image = server.name;
}
let instance = await this.instanceRepo.create({
serverId,
status: 'STARTING',
});
try {
const spec: ContainerSpec = {
image,
@@ -265,7 +365,7 @@ export class InstanceService {
hostPort: null,
network: MCP_SERVERS_NETWORK,
labels: {
'mcpctl.server-id': serverId,
'mcpctl.server-id': server.id,
'mcpctl.instance-id': instance.id,
},
};
@@ -283,7 +383,17 @@ export class InstanceService {
}
}
// Resolve env vars from inline values and secret refs
// Resolve env vars from inline values and secret refs.
//
// Failure here is FATAL for the start attempt: a container that
// boots without its declared secrets will silently mis-behave (we
// saw this with gitea-mcp-server starting up with an empty
// GITEA_ACCESS_TOKEN when OpenBao was unreachable, then reporting
// "healthy" while every authed call failed). Marking the instance
// ERROR with a backoff-aware nextRetryAt is honest; the reconciler
// will retry it in-place on the next tick whose nextRetryAt has
// elapsed. Optional/missing env vars should be modeled as `value: ""`
// entries on the server, not as silent secret-resolution failures.
if (this.secretResolver) {
try {
const resolvedEnv = await resolveServerEnv(server, this.secretResolver);
@@ -291,8 +401,8 @@ export class InstanceService {
spec.env = resolvedEnv;
}
} catch (envErr) {
// Log but don't prevent startup — env resolution failures are non-fatal
// The container may still work if env vars are optional
const msg = envErr instanceof Error ? envErr.message : String(envErr);
return this.markInstanceError(instance, `secret resolution failed: ${msg}`);
}
}
@@ -313,14 +423,39 @@ export class InstanceService {
}
// Set STARTING — syncStatus will promote to RUNNING once the container is actually ready
instance = await this.instanceRepo.updateStatus(instance.id, 'STARTING', updateFields);
return this.instanceRepo.updateStatus(instance.id, 'STARTING', updateFields);
} catch (err) {
instance = await this.instanceRepo.updateStatus(instance.id, 'ERROR', {
metadata: { error: err instanceof Error ? err.message : String(err) },
});
return this.markInstanceError(
instance,
err instanceof Error ? err.message : String(err),
);
}
}
return instance;
/**
* Mark an instance ERROR with a backoff-aware retry schedule. The
* `attemptCount` accumulates across retries (preserved by
* `retryInstance` which reuses the same row), so the schedule
* actually escalates: 30s × 5 → 5min thereafter.
*/
private async markInstanceError(
instance: McpInstance,
error: string,
): Promise<McpInstance> {
const meta = readRetryMeta(instance);
const attemptCount = (typeof meta.attemptCount === 'number' ? meta.attemptCount : 0) + 1;
const delayMs = nextDelayMs(attemptCount);
const now = new Date();
const nextRetryAt = new Date(now.getTime() + delayMs).toISOString();
return this.instanceRepo.updateStatus(instance.id, 'ERROR', {
metadata: {
...meta,
error,
attemptCount,
lastAttemptAt: now.toISOString(),
nextRetryAt,
},
});
}
/** Stop and remove a single instance. */