fix: dedupe MCP server entries and retry sidecar connections on startup - #60
Merged
Conversation
Two MCP wiring reliability issues surfaced while debugging agent chat: 1. Duplicate MCP_SERVERS entries: the controller builds MCP_SERVERS from Agent.spec.enabledMCPs discovery and AgentImage.spec.mcpServers independently, so an MCP declared on both (e.g. mem0) reached the runtime twice and was connected/registered twice. Dedupe by server name (first occurrence wins) before emitting the env var, and drop duplicates at parse time in the pi extension as well. 2. Startup race: pi-ainsel-mcp connected to each server exactly once at process start; if a pod sidecar (e.g. chat on localhost:8081) was not listening yet, that server was skipped for the pod's entire lifetime and the agent silently lost its tools. Retry per server with exponential backoff (4 attempts, 250ms base) inside the existing startup budget.
Owner
Author
|
@review-agent please have a look |
There was a problem hiding this comment.
Review Summary
Overall: Approve
Focused, well-diagnosed fix for two real MCP wiring bugs; the operator-side dedupe is properly unit-tested, the runtime-side dedupe is placed correctly after validation, and the retry loop stays inside the existing startup budget. CI fully green on the head SHA (5/5 runs).
Blockers
- None.
Suggestions
pi/pi-extensions/ainsel-mcp/catalog.ts:84— The retry loop never re-checksdeadline.abortedafterawait sleep(delayMs), andsleepitself is not deadline-aware. Worst case,connect()starts one extra attempt after the startup budget has expired and overshoots it by up to the last backoff delay (~2s). Bounded and harmless in practice, but adeadline.abortedcheck after the sleep would make the 10s budget strict.pi/pi-extensions/ainsel-mcp/catalog.ts:62/parse.ts:35— No checked-in automated tests for the retry loop or the runtime dedupe (the PR body documents manual Node smoke tests for both). This area of the repo has no TS test harness and CI only runsnode --check, so this is not blocking — but the async backoff loop is exactly the kind of code that regresses silently; worth considering a minimal test setup for the extension as follow-up work.pi/pi-extensions/ainsel-mcp/parse.ts:35— The runtime-side dedupe drops duplicates silently. A debug-level log line ("duplicate MCP server entry dropped") would make future operator-side regressions easier to spot in pod logs.
Nits
operators/agent/internal/controller/mcpservers/discover.go:79—DedupeEntrieskeys on the untrimmed prefixe[:i]whileparseServerstrims before deduping. The controller never emits whitespace-padded entries so this can't diverge in practice — just noting it for anyone tempted to reuseDedupeEntrieson arbitrary input.
What's Done Well
- Defense in depth with matching semantics: dedupe at the source (operator) and defensively at runtime (
parseServers), both first-wins. - Retry logic cleanly extracted into
connectServer, with each attempt observable — both the retry and the final skip log lines carry attempt metadata. - Good test coverage for
DedupeEntries(first-wins order, nil input, no-=entries); thei > 0guard handles leading-=gracefully. - Excellent PR description: root cause, observed symptoms, per-side fix, verification, and deployment notes (AgentImage tag bump on the dev cluster).
Testing
- Operator: three new unit tests in
discover_test.gocover the new function;go test+ lint pass in CI. - Pi extension: no checked-in tests (no TS harness exists in the repo); the PR documents smoke tests covering dedupe and the retry/recovery path, and CI (
node --check, both image builds, secret scan) is green on headdc7b680.
|
@DominikPinsel LGTM — approved. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two MCP wiring reliability issues surfaced while debugging agent chat (fix in #59 + deployment PR AInsel/ainsel-deployment#14):
1. Duplicate
MCP_SERVERSentriesThe controller builds
MCP_SERVERSfromAgent.spec.enabledMCPsdiscovery andAgentImage.spec.mcpServersindependently, so an MCP declared on both (e.g.mem0on the dev cluster) reached the runtime twice and was connected/registered twice:Fix:
mcpservers.DedupeEntries(first occurrence wins) applied before emitting the env var, plus defensive first-wins dedupe in the pi extension'sparseServers.2. Startup race — sidecar connections skipped forever
pi-ainsel-mcpconnected to each server exactly once at process start. If a pod sidecar (e.g. chat onlocalhost:8081) was not listening yet, the server was skipped for the pod's entire lifetime:Observed live on
agent-a-14b9fb30after a rollout; a pod restart "fixed" it by winning the race.Fix: per-server retry with exponential backoff in
Catalog.connect(4 attempts, 250ms base: 0.25+0.5+1+2s), bounded by the existing 10s startup budget. Each attempt is logged (connect failed; retrying).Verification
go test ./internal/controller/mcpservers/+go build ./...(operators/agent) ✅golangci-lint run ./internal/controller/...— 0 issues ✅connected, tool registered) ✅Deployment notes
devtag, digest-aware restarts).dpinsel/ainsel-pi-goagent image; the dev cluster currently pins1.24via its AgentImage CRs, so those need a tag bump to pick this up.