fix(mcp): harden stdio transport recovery

[silverstein]

Summary

Harden stdio MCP lifecycle handling so dead children trigger a fresh transport without coroutine leaks, PID cross-contamination, event-loop restart failures, or optional-ping reconnect churn.

Slack deduplication is intentionally split into focused PR #95124.

Changes

• Retire stale stdio sessions and signal reconnect on pre-call or mid-call child death without replaying potentially non-idempotent tools.
• Serialize only stdio spawn/PID attribution with a loop-neutral guard while leaving established sessions concurrent.
• Use owned-process liveness for local stdio idle checks; retain protocol ping/list-tools keepalive for HTTP/SSE.
• Keep the child watcher coroutine-specific and construct it exactly once.
• Add regressions for pre-call death, mid-call death, fresh-session retry, overlapping server spawns, event-loop recreation, and stdio no-ping behavior.

Keepalive rationale

Local stdio transports have no remote session TTL, and observed otherwise-valid SDK servers close when sent the optional MCP ping. Stdio therefore defaults to owned-process liveness at idle and proves protocol health through bounded real calls. HTTP/SSE retains protocol keepalive because remote session expiry cannot be inferred from a local process. An opt-in stdio protocol-probe mode can be considered separately without restoring a default known to churn valid servers.

Verification

• pytest tests/tools/test_mcp*.py → 591 passed
• Ruff on all changed files → passed
• git diff --check origin/main...HEAD → passed

Risk / rollback

Scoped to MCP transport lifecycle and tests. Revert the six PR commits to restore prior behavior.

[Enough1122]

AI code review — automated review for reference; please use your judgment.

• plugins/platforms/slack/adapter.py:4949 — _handle_slack_file_shared now resolves its client through _canonical_event_team_id instead of _event_team_id, and the canonicalizer's len(known_teams) == 1 shortcut (adapter.py:4907) attributes events from ANY unknown/Enterprise-scoped team to the single authenticated workspace. In a one-workspace deployment where a Slack-Connect event arrives scoped to the partner org, the old code returned "" and fell back to _get_client(channel_id) (which can resolve via _channel_team); the new code returns the sole known team and calls files_info with that workspace's client/token for a file the workspace may have no access to — a wrong-token API error at best, a cross-tenant fetch attempt at worst. Consider restricting the single-team shortcut to the dedup-id computation, or verifying the channel actually maps to the known team before using its client.
• tools/mcp_tool.py:3084-3095 — Removing the protocol keepalive for stdio entirely means a wedged but alive stdio server (deadlocked event loop, stuck on blocked stdin — process alive, protocol dead) is now invisible at idle: _stdio_children_dead() is a pure liveness probe, and the comment's "protocol health is proved by real tool calls" only helps when traffic exists. An idle-then-wedged stdio session will sit "healthy" until the next user call times out — arguably a worse UX than the old behavior for servers that DO tolerate ping. If ping is unsafe for known-bad servers, consider gating the protocol probe by server config (keepalive_protocol: ping|list_tools|off) rather than dropping it for all stdio servers.
• tools/mcp_tool.py:6203-6207 — Good fix on the double _watch_children() invocation (previously one unawaited watcher coroutine per call — real coroutine-leak). One residual nit: candidate = _watch_children() followed by inspect.isawaitable(candidate) still creates the coroutine before rejecting it, so a non-coroutine-returning stub is fine, but an awaitable that isn't a coroutine (e.g. a Future) is accepted by isawaitable yet asyncio.ensure_future handles it differently than the surrounding asyncio.wait bookkeeping expects — the old code required iscoroutine; consider keeping that restriction for the watcher too.

[silverstein]

Thanks for the review. I made two structural changes and addressed the concrete implementation points:

• The Slack deduplication work is now split into focused PR fix(slack): deduplicate cross-scope message events #95124, so this PR is MCP-only. The file_shared client-routing concern is fixed and regression-tested there by keeping canonical team normalization out of auth/client selection.
• The child watcher is coroutine-specific again, while still being constructed exactly once, so the earlier unawaited-coroutine leak stays fixed without broadening the accepted watcher shape.

I am intentionally retaining process liveness as the default idle probe for stdio. Local stdio has no remote session TTL, and we reproduced valid SDK servers closing their transports when sent the optional MCP ping. Real calls remain bounded and prove protocol health; child death is detected before and during calls. HTTP/SSE still uses ping with list_tools fallback because remote session expiry cannot be inferred from a local process. An opt-in stdio protocol-probe mode could be considered separately without restoring a default known to churn valid servers.

Verification on the rebased MCP-only head: 591 MCP tests passed, Ruff passed, and git diff --check passed.

[kshitijk4poor]

Heads-up: the _stdio_children_dead polarity fix within this PR's scope landed on main today via #94339 (merge commit ef46ec0). The broader transport-recovery hardening here (stale-session retirement, PID cross-contamination, loop-restart) remains unique — please rebase onto current main so the overlap drops out of the diff.

[kshitijk4poor]

Triage note (not a review verdict): this PR bundles several independent pieces, two of which have since landed or been salvaged elsewhere — the _stdio_children_dead polarity fix is on main via #94339, and reconnect-on-fast-fail is being salvaged in #96452 (from #95626, which reached the same conclusion). The remaining unique pieces here — the _tracked_stdio_spawn spawn-lock/PID-attribution fix and the stdio idle-probe redesign (process liveness instead of protocol ping) — are both substantive and worth their own focused PRs. Suggest splitting so each can be reviewed on its own merits; happy to help salvage if useful.

[silverstein]

Rebased onto current main at 42ac29eacc, as requested.

The overlapping _stdio_children_dead polarity/probe change from #94339 is now absent from this PR's diff, along with its duplicate liveness tests. The first commit now retains only the unique single-construction fix for the child-watcher coroutine. The remaining stale-session retirement, PID attribution isolation, loop-neutral spawn locking, stdio keepalive behavior, and HTTP keepalive coverage remain intact.

Final verification on head c2314b9383:

• 598 MCP tests passed, 0 failed
• Ruff passed
• git diff --check passed
• post-rebase range-diff showed all six retained patches identical after the final upstream update
• GitHub now reports the PR mergeable

[silverstein]

Split completed as requested:

• fix(mcp): isolate concurrent stdio PID attribution #96469 contains only the unique stdio spawn-lock/PID-attribution fix.
• fix(mcp): use process liveness for stdio idle probes #96470 contains only the unique stdio idle-probe redesign.
• fix(mcp): request a respawn when the stdio fast-fail gate finds a dead subprocess (salvage #95626) #96452 remains the focused reconnect-on-fast-fail salvage.
• fix(mcp): un-invert the stdio children liveness check #94339 already carries the child-liveness polarity/probe fix.

Both replacement PRs are rebased onto current main, independently mergeable, and include focused behavioral tests plus full MCP-suite verification. I am closing this broad PR to remove the overlapping review surface. Thanks for the clear triage and split recommendation.