85e53e8d1e16b5715f00c9ae20503238864f7311
5 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Michal
|
b022f322f0 |
fix(mcpd): recover k8s STDIO instances after in-place container restarts
A container restart (OOMKill, crash, transient npx failure) used to strand the instance in ERROR forever while its pod sat 1/1 Running, because five gaps lined up (#114): - the exec/attach websocket died without ending the stdout PassThrough (client-node installs no onclose), so PersistentStdioClient — whose only death signal was stdout 'end' — kept believing it was connected and every request rode the 120s timeout into a dead pipe; - the stdioClients cache is keyed by pod name, which survives a restart, so nothing ever evicted the corpse; - syncStatus never re-inspected ERROR rows and never read restartCount, so neither the recovery nor the in-place restart was visible; - its ERROR writes clobbered retry metadata, making the row instantly dueForRetry, and the retry recreated the pod under the SAME name — an uncaught 409 that looped ERROR against a healthy pod; - the stuck row consumed the whole replica budget, blocking a fresh-id replacement. The fix, layer by layer: - ws 'close'/'error' now end stdout in both k8s interactive paths (and the docker interactive path mirrors its own one-shot handlers), funneling into an identity-guarded teardown in PersistentStdioClient that also listens for stream 'close'/'error' — a late event from a previous session cannot clobber a reconnected one; - syncStatus re-inspects ERROR rows (pod running again → back to RUNNING with retry metadata cleared), tracks restartCount in instance metadata to catch restarts between polls, MERGES retry metadata instead of clobbering it, and evicts the cached stdio client through a setter-injected hook whenever the pipe is known-dead; - createContainer adopts an alive pod on 409 instead of throwing (and only replaces a genuinely dead one, waiting out the deletion grace period); - attach mode retries once through a fresh client before surfacing an error, so the first call after a detected death succeeds; - the health probe evicts the cached client when failures cross the threshold, and shutdown finally calls closeAll(). Closes #114 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JaFvfHrQyUKCGv6o3N2Wir |
||
|
Michal
|
b2547429ca |
fix(health): a passing tools/list is live, not healthy
Some checks failed
CI/CD / lint (pull_request) Successful in 1m16s
CI/CD / test (pull_request) Successful in 1m29s
CI/CD / typecheck (pull_request) Successful in 3m15s
CI/CD / smoke (pull_request) Failing after 2m0s
CI/CD / build (pull_request) Successful in 5m5s
CI/CD / publish (pull_request) Has been skipped
`mcpctl get instances` showed all eight servers healthy while the UniFi one
had never once reached its controller. The default probe is `tools/list`,
which MCP servers answer from a static in-process table — no credentials, no
upstream, ~3ms. It cannot fail for any reason the user cares about, so it was
reporting `healthy` for every process that managed to start.
Split the two passes:
healthy — readiness: `tools/call` on `healthCheck.tool`. The upstream
answered, so the server can actually do its job.
live — liveness: `tools/list` only. Process up, upstream unverified.
`live` is now the default for any server without a `healthCheck.tool`. It is
not a warning; it is an admission that nothing is watching that server. Probe
events name which probe ran and which tool ("Readiness check (list_sites)
passed"), so the events log distinguishes the two after the fact.
Also:
- `healthCheck.tool` is optional now, so the timings can be tuned without
inventing a readiness probe.
- `create server --health-check-tool/-args/-interval/-timeout/
-failure-threshold`, per the rule that everything applyable is a create
flag. Merges over a `--from-template` healthCheck rather than replacing it.
- `describe instance` explains a `live` verdict instead of leaving it cryptic.
- create.ts held a raw NUL byte in a string literal, which made grep treat the
whole file as binary and silently skip it. Escaped as `\0`.
Verified against the live fleet: with readiness probes configured, my-grafana
went unhealthy (Grafana API 403) and my-node-red degraded (connect timeout to
a Tailscale address) — both had read healthy for months.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0114dg56YmVacyqhp5fitcTb
|
||
|
Michal
|
e6cd73543a |
fix(mcpd): fail-loud on env resolution + retry/backoff + readiness via proxy
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>
|
||
|
Michal
|
3149ea3ae7 |
fix: MCP proxy resilience — discovery cache, default liveness probes
Some checks failed
Adds a per-server tools/list cache in McpRouter (positive + negative TTL) so a slow or dead upstream only stalls the first discovery call, not every subsequent client request. Invalidated on upstream add/remove. Health probes now apply a default liveness spec (tools/list via the real production path) to any RUNNING instance without an explicit healthCheck, so synthetic and real failures converge on the same signal. Includes supporting updates in mcpd-client, discovery, upstream/mcpd, seeder, and fulldeploy/release scripts. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Michal
|
738bfafd46 |
feat: MCP health probe runner — periodic tool-call probes for instances
Implements Kubernetes-style liveness probes that call MCP tools defined in server healthCheck configs. For STDIO servers, uses docker exec to spawn a disposable MCP client that sends initialize + tool call. For HTTP/SSE servers, sends JSON-RPC directly. - HealthProbeRunner service with configurable interval/threshold/timeout - execInContainer added to orchestrator interface + Docker implementation - Instance findById now includes server relation (fixes describe showing IDs) - Events appended to instance (last 50), healthStatus tracked as healthy/degraded/unhealthy - 12 unit tests covering probing, thresholds, intervals, cleanup Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> |