fix: retry an MCP server's first connect attempt before giving up (#610)

[MCKRUZ]

Summary

Closes #610. McpConnectionManager.CreateClientAsync had no retry on a server's first connect
attempt — only an already-connected client that later went stale got a retry. A single slow
cold-start (npx still installing, a container still starting) on the one server a plugin's
DeniedTools entry depends on could permanently deny that plugin's entire tool surface for the
life of the host, since PluginBoundaryStatus.Faulted has no un-fault path by design.

Retries the whole connect (fresh transport per attempt) up to 3 times with a jittered 1s delay,
scoped to a genuine first connect only.

Review history — round 2 found 8 real bugs in the fix itself

A 5-angle background /code-review pass on the initial fix found, and I fixed (each verified
empirically against the pinned SDK and mutation-tested):

1. Multiplicative retry — PluginToolBoundaryStartupValidator's own 5-attempt availability
   probe wrapped this method too, multiplying worst-case attempts to 15 and wall-clock to ~3x.
   That outer loop existed only to compensate for the connect layer having no retry of its own —
   removed entirely rather than tuned around.
2. Reconnect latency regression — applying the same retry budget to ReconnectAsync (a live
   agent turn recovering an already-established, stale session) would have tripled worst-case turn
   latency for a scenario Plugin boundary: initial MCP connect has no retry, so a slow cold-start permanently Faults the plugin #610 was never about. Scoped retry to first-connect only.
3. Cancellation misreported as a connection failure — a genuine cancellation landing on the
   retry loop's last attempt was silently wrapped into McpConnectionException (no subsequent
   delay call to re-surface it, unlike earlier attempts) — could spuriously record a permanent
   Faulted status for a server that was never actually unreachable.
4. Deterministic config error burning the retry budget — an empty Stdio Command throws a raw
   ArgumentException from the SDK's own options setter, which the retry filter treated as
   transient. Now validated explicitly, matching the HTTP "missing URL" check.
5. Function-length violation — extracted the retry loop into its own method.
6. Thundering herd — added jitter so multiple cold-starting servers don't retry in lockstep.
7. Bundle-owned audit duplication — a bundle-owned connection is individually audited per
   request; retrying a deterministic security verdict (an egress-policy deny or an AntiSSRF block
   against a fixed target) wrote duplicate audit entries for one decision. Bundle-owned connections
   are now never retried, regardless of caller intent.
8. Grader-flagged residual function length — moved the retry filter's exclusion rationale into
   XML remarks after the fix above still left the method a few lines over the guideline.

Test plan

• dotnet build src/AgenticHarness.slnx
• Infrastructure.AI.MCP.Tests: 156/156 pass
• Infrastructure.AI.Tests (full suite): 3523/3523 pass (one known pre-existing flaky
  sandbox-process test confirmed clean in isolation)
• Every fix mutation-tested (revert → confirm test fails → restore → confirm passes)
• Verified empirically against the pinned SDK (ModelContextProtocol 1.4.1) via throwaway
  console apps — process/transport disposal on a failed connect, cancellation exception shape,
  the Stdio ArgumentException shape — not asserted from documentation alone
• run-gates.sh grader gate: clean pass ("LOOKS GOOD", one non-blocking function-length note,
  since fixed). Local full-suite run-gates.sh repeatedly killed by genuine host memory
  pressure (not test failures) — pushed via the sanctioned RAILS_SKIP_REVIEW_GATE=1 bypass,
  deferring to CI's non-contended run.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Aao7Y3hU22v6VH1RdiSxYu

[github-actions]

🔒 Security review — PR #650

Bottom line: PASS — no HIGH findings. The change adds a bounded retry (3 attempts, 1s + ≤250ms jitter) to a first MCP connect and removes the now-redundant outer availability-probe loop in PluginToolBoundaryStartupValidator. I traced every security-relevant control the retry loop now re-enters and found no bypass, no fail-open, and no new credential/audit exposure.

What I verified (no finding)

• SSRF / metadata-endpoint guard is re-run on every attempt, not hoisted. ConnectWithRetryAsync builds a fresh transport inside the loop, so CreateTransport → CreateHttpTransport → ValidateMcpServerUrl (scheme allowlist + BlockedHosts IMDS check) executes per attempt, and the shared _antiSsrfHandler still performs connect-time IP filtering on each connection — retry gives no extra DNS-rebinding leverage, since the check is per-socket, not per-Uri.
• Both SSRF rejections are excluded from retry. ValidateMcpServerUrl throws McpConnectionException, which the catch filter (ex is not McpConnectionException) deliberately does not retry — a blocked host fails once, fast, as before.
• Audit integrity for bundle-owned egress is preserved. retryOnFirstConnect && !isBundleOwned in CreateClientAsync:697 means a bundle-owned (uploader-declared, untrusted) server is never retried, so EgressPolicyDelegatingHandler still writes exactly one audit entry per allow/deny decision. This control has a real failing test behind it: BundleMcpEgressAttributionTests.GetClientAsync_BundleOwnedServerOnDisallowedHost_BlocksAndAuditsDenyDecision asserts audit.Entries.Should().ContainSingle() and would fail (3 entries) if !isBundleOwned were deleted.
• No fail-open in the plugin boundary. Excluding OperationCanceledException from the wrap-into-McpConnectionException path changes what a cancelled connect reports, so I checked the resulting state: an unreported server stays PluginBoundaryStatus.Pending, documented and implemented as deny-all (PluginBoundaryStatus.cs:20-22). The more permissive-looking exception change therefore lands on the fail-closed side. A non-caller-cancellation OCE still falls through McpToolProvider's when (cancellationToken.IsCancellationRequested) filter into catch (Exception) → SafeReportDiscoveryToBoundaryTracker(serverName, []) → fault. Same for PluginToolBoundaryStartupValidator, which passes no token.
• Removing the 5× availability probe does not weaken boundary verification. The probe was side-effect-free; the single reporting GetToolsAsync call is unchanged, and its connect now retries internally. Fewer total attempts (3 vs 5×1), still fail-closed on exhaustion.
• No client/handler leak from retry. _entraClients and _bundleEgressClients are GetOrAdd-cached, so a retried connect reuses the same credential-bearing HttpClient rather than allocating one per attempt.
• Amplification is bounded and applies only to operator-configured servers. Worst case is 3 × StartupTimeoutSeconds holding a per-server semaphore; untrusted bundle definitions are excluded entirely.

MEDIUM / LOW — advisory, not blocking

LOW · McpConnectionManager.cs:982 — a malformed Url still burns the full retry budget.
new Uri(definition.Url) throws UriFormatException (a FormatException, not McpConnectionException), so an HTTP/SSE server with a syntactically invalid URL is treated as transient and retried 3× with ~2s of pure delay. This is the identical class of deterministic config error the round-2 fix just closed for Stdio's empty Command at line 784, so it's an inconsistency in a fix that otherwise handles it. Suggested fix: wrap the parse in Uri.TryCreate and throw McpConnectionException($"MCP server '{serverName}' has a malformed URL."), mirroring the existing missing-URL and missing-Command checks. (Per this repo's own "fixing one instance of a duplicated pattern and stopping there" rule, this is the sibling occurrence worth closing in the same pass.)

LOW · McpConnectionManager.cs:735-743 — the OCE exclusion is unconditional on the caller's token.
The filter excludes every OperationCanceledException, not just caller-requested cancellation. The remarks assert a timed-out handshake is retried, but if the pinned SDK surfaces its own InitializationTimeout as a TaskCanceledException (the usual shape for a CancellationTokenSource-driven timeout), the retry is inert for the exact cold-start-timeout case #610 targets. No security consequence — the boundary stays Pending/Faulted, i.e. deny-all either way — but the availability fix may not cover its headline scenario. Narrowing the filter to and not OperationCanceledException when cancellationToken.IsCancellationRequested (or an explicit ex is OperationCanceledException && cancellationToken.IsCancellationRequested rethrow) would make intent match behaviour. I could not restore the MCP package in this environment to confirm which exception the pinned SDK actually throws; the PR comments claim empirical verification, so treat this as a prompt to re-check rather than a confirmed defect.

LOW · McpConnectionManager.cs:744-747 — new per-attempt LogWarning(ex, ...) on the connect path.
Intermediate failures now log the raw exception, which for HTTP transports typically carries the request URI. An operator who embedded a credential in an MCP server URL would see it in warning-level logs. Mitigated by the existing local log-redaction layer (#457/#499), so noting only.

Reviewed: McpConnectionManager.cs, PluginToolBoundaryStartupValidator.cs, and both test files, plus the ValidateMcpServerUrl / ResolveTransportHttpClient / ResolveBundleEgressClient / PluginBoundaryStatus / McpToolProvider reporting paths reached from them.

[github-actions]

Correctness review — PR #650

Bottom line: CORRECT — no blocking defects found in the anchor set. Five advisories below; the one worth a human's eyes is #1 (a code comment that asserts behaviour the code never performs).

Blocking defects

None.

What I verified (so the PASS is legible)

• ConnectWithRetryAsync's loop is bounded correctly: maxAttempts >= 1, the final iteration always returns or throws, UnreachableException is genuinely unreachable, and using System.Diagnostics; was added (McpConnectionManager.cs:2).
• The new OperationCanceledException exclusion (McpConnectionManager.cs:754) does not break the only consumer chain: McpToolProvider.TryConnectAsync catches Exception broadly, so an OCE escaping GetClientAsync still collapses to the same null the old wrapped McpConnectionException did. There is no catch (McpConnectionException anywhere in production code.
• Removing the outer probe loop (PluginToolBoundaryStartupValidator.cs:219-229) is compensated: McpToolProvider.GetToolsAsync reaches _connectionManager.GetClientAsync (McpToolProvider.cs:98), which now carries the retry. IsServerAvailableAsync still has a live caller (McpToolsExample.cs:146) so nothing is orphaned.
• The bundle-owned retry suppression (McpConnectionManager.cs:696) is correctly scoped: only ResolveBundleEgressClient attaches EgressPolicyDelegatingHandler; the host-configured path uses the shared _httpClient with no audit writer, so retrying a host server cannot duplicate egress audit entries.
• Repeated CreateTransport calls across attempts leak nothing: both _entraClients and _bundleEgressClients are GetOrAdd caches.
• ReconnectAsync(serverName, null!, token) in the new test is safe — _clients.TryGetValue returns false for an uncached name, so ReferenceEquals(current, failedClient) short-circuits (McpConnectionManager.cs:457).
• Log template at McpConnectionManager.cs:763 — 4 placeholders, 4 args, correct order.

Advisory

1. src/Content/Infrastructure/Infrastructure.AI.MCP/Services/McpConnectionManager.cs:241 — the comment says retryOnFirstConnect: true because "this IS a first connect", but this path is unreachable-with-retry: RequiresRunScope returns true only for a server in _bundleOwnedServers with Type == Stdio, and CreateClientAsync:696 ANDs the flag with !isBundleOwned. So run-scoped connects never retry, and the comment documents the opposite. Either drop the comment's claim or state the suppression explicitly — a future reader trusting it will mis-model the budget.

2. McpConnectionManager.cs:784 — the new guard uses string.IsNullOrEmpty(definition.Command). A whitespace-only Command (" ") passes it and reaches StdioClientTransportOptions's setter, which (per the guard's own premise) throws a raw ArgumentException — which is not excluded from retry, so it burns the full ~2s budget on exactly the deterministic config error this guard was added to fail fast on. IsNullOrWhiteSpace closes it. (Not blocking: wasted latency only, no wrong result.)

3. McpConnectionManager.cs:280-289 — the new remarks dismiss the widened lock hold as "Not a correctness change — ConcurrentDictionary.TryRemove is still atomic either way." Atomicity isn't the hazard. This same file (DisconnectAsync's remarks) states what the lock actually buys: "exclusion against a concurrent CreateAndCacheClientAsync caching a NEW client for this server between this method's remove and its return." Tripling the hold makes the >5s DisconnectLockTimeout fallback materially more reachable, and on that path the in-flight connect's _clients[serverName] = client can resurrect an entry a disconnect just evicted — a live, never-disposed session (and stdio child process) surviving teardown. Advisory rather than blocking because the gap is pre-existing (this PR only widens the window) and DisconnectAsync's bundle-teardown caller targets bundle-owned servers, which this PR excludes from retry entirely. Still: the remark's stated justification does not cover the failure mode its own sibling doc names.

4. McpConnectionManager.cs:754 — the OCE exclusion is correct only if the pinned SDK maps an InitializationTimeout expiry to TimeoutException rather than letting OperationCanceledException/TaskCanceledException escape. I could not verify this in the review environment (the ModelContextProtocol 1.4.1 package is not restored here), and the doc's "Everything else — a timed-out handshake ... is exactly the transient cold-start shape this retry exists for" is the single load-bearing assumption of the whole change: if that shape is an OCE, the most common cold-start failure is silently excluded from retry and Plugin boundary: initial MCP connect has no retry, so a slow cold-start permanently Faults the plugin #610 is not actually fixed for it. A regression test asserting a handshake timeout (not a fast process exit) retries would pin this — the new ..._FirstConnectAttemptFails_RetriesBeforeThrowing test covers the exit 1 shape only.

5. src/Content/Tests/Infrastructure.AI.MCP.Tests/Services/McpConnectionManagerExtendedTests.cs:201-321 — three timing-coupled assertions on shared CI (> 1800ms, < 500ms, and a 300ms CancelAfter that must land inside McpClient.CreateAsync after a process spawn). The < 500ms fail-fast bound is the most exposed — it asserts that a throw-only path beats half a second on a machine that may be heavily contended. Consider widening the fail-fast bound (anything well under ConnectRetryDelay proves the same thing).

6. src/Content/Infrastructure/Infrastructure.AI/Plugins/PluginToolBoundaryStartupValidator.cs:219 — net attempt budget before a permanent PluginBoundaryStatus.Faulted drops from 5 non-reporting probes + 1 reporting attempt to 3 attempts. The comment argues the outer loop "no longer adds resilience", which is true of the multiplication but not of the total; if Plugin boundary: initial MCP connect has no retry, so a slow cold-start permanently Faults the plugin #610's intent was to raise cold-start tolerance, confirm 3 is enough for the npx-cold-start case that motivated the original loop.