fix(mcp): serialize keepalive with active RPCs

[0xquinto]

What does this PR do?

Prevents MCP keepalive probes from overlapping an active RPC on the same ClientSession/JSON-RPC stream.

When _rpc_lock is already held, the keepalive cycle now returns immediately because active RPC traffic already proves liveness. Otherwise, the probe acquires the same per-server lock used by call_tool() and the other client-initiated RPCs before sending ping or its list_tools fallback. This closes the check/acquire race without queueing the common keepalive path behind a long-running tool call.

The bounded trade-off is the opposite ordering: if keepalive acquires the lock first against a dead server, a newly arriving user RPC waits behind the probe's existing 30-second timeout before its own timeout starts. The regression suite covers that serialization order explicitly. When a user RPC owns the lock and is itself hung, its own timeout remains the failure detector while the redundant periodic probe stays silent.

The bug was reproduced with a long-running stdio MCP call: a concurrent ping completed while the original tools/call remained wedged until the outer timeout. A fresh-process live canary after the fix completed the real call in 10.546 seconds, skipped keepalive during the active call, and completed a post-call keepalive.

Relationship to #48069

#48069 identified the same in-flight keepalive race and proposed a broader fix that also tracks and cancels orphaned calls during reconnect. Credit to @arminanton for documenting that failure mode first.

That branch is currently 2,923 commits behind main and has a content conflict in tools/mcp_tool.py. This PR is a narrow current-main salvage of only the serialization invariant, with a direct regression in the existing capability-gating suite. It intentionally does not duplicate the broader reconnect/orphan-cancellation behavior from #48069.

Related Issue

Related to #48069 and #30268.

Type of Change

• 🐛 Bug fix (non-breaking change that fixes an issue)
• ✅ Tests (adding or improving test coverage)

Changes Made

• tools/mcp_tool.py
  • skip the periodic probe when _rpc_lock already indicates active traffic;
  • serialize both ping and the list_tools fallback with _rpc_lock;
  • document the load-bearing all-client-RPC lock invariant and lifecycle ownership of the session snapshot;
  • snapshot the active session and fail explicitly if no session exists.
• tests/tools/test_mcp_capability_gating.py
  • prove keepalive returns promptly and emits neither ping nor list_tools while another RPC owns the lock;
  • prove a user RPC waits when keepalive owns the lock.

How to Test

1. Focused regression:

   uv run --extra dev pytest -q tests/tools/test_mcp_capability_gating.py::TestKeepaliveProbe::test_keepalive_skips_an_active_tool_rpc tests/tools/test_mcp_capability_gating.py::TestKeepaliveProbe::test_active_keepalive_serializes_a_user_rpc -o 'addopts='
   

2. Targeted MCP suites:

   uv run --extra dev pytest -q tests/tools/test_mcp_tool.py tests/tools/test_mcp_stability.py tests/tools/test_mcp_capability_gating.py -o 'addopts='
   

3. Static checks:

   uv run --extra dev python scripts/check-windows-footguns.py --all
   uv run --extra dev ruff check tools/mcp_tool.py tests/tools/test_mcp_capability_gating.py
   

Observed locally on macOS 26.5.2 / Apple Silicon:

• focused test: RED before the skip guard, GREEN after;
• targeted MCP suites: 265 passed;
• Windows footgun scan: 756 files, no findings;
• ruff and py_compile: passed;
• fresh-process real MCP/Fable canary: status=ok, 10.546 s, keepalive skipped during the call, post-call keepalive completed.

I also attempted the repository-wide scripts/run_tests.sh in the managed local environment. It did not produce a clean baseline: 39 failures were reported across 17 unrelated test files, 10 files did not run, and tests/tools/test_browser_homebrew_paths.py hit the wrapper's 300-second per-file timeout. None of the failures were in the three targeted MCP suites above, so this PR does not claim a clean local full-suite run; fork CI is the authoritative full check.

Checklist

Code

• I've read the Contributing Guide
• My commit message follows Conventional Commits
• I searched existing issues and PRs and documented the relationship to fix(tools): skip MCP keepalive during in-flight calls + fail orphaned calls on reconnect #48069
• My PR contains only the two files related to this fix
• I've run a clean complete pytest tests/ -q suite locally (repository-wide wrapper was attempted but had unrelated local failures/timeouts; targeted 265-test MCP suite passed; full CI is expected to run on the PR)
• I've added a regression test
• I've tested on macOS 26.5.2 / Apple Silicon

Documentation & Housekeeping

• Relevant behavior is documented in inline comments; no public config or schema changes
• cli-config.yaml.example: N/A
• CONTRIBUTING.md / AGENTS.md: N/A
• Cross-platform impact considered; the change uses existing asyncio.Lock semantics and passes the Windows-footgun scanner
• Tool descriptions/schemas: N/A

[teknium1]

Thanks for the focused fix. Current main still sends keepalive ping / fallback list_tools outside the per-server RPC lock (tools/mcp_tool.py:1840-1862), while normal tool calls are serialized by that lock (tools/mcp_tool.py:3934-3945). The PR applies the existing invariant to both keepalive paths and adds focused ordering coverage.

No blocking problems identified in the reviewed diff.

This is an automated hermes-sweeper review.

[0xquinto]

Operational note: while this PR is pending, I applied a per-server workaround in the affected deployment:

mcp_servers:
  fable-advisor:
    keepalive_interval: 86400

and reloaded the MCP servers.

This only makes scheduled probes rare for that server. It is not a correctness fix: a probe can still coincide with a long call, and it weakens idle liveness detection. keepalive_interval: 0 is not a disable switch because it is clamped to _MIN_KEEPALIVE_INTERVAL.

The PR remains the durable fix because it enforces the no-overlap invariant while preserving idle keepalives for all MCP servers.

[0xquinto]

Checking in after ~12 days. Still mergeable against today's main (d9165d7), and the gap is still live after the recent recycle/backoff work: _keepalive_probe() sends ping/list_tools outside _rpc_lock (L2142–L2180; loop call at L2280). Filed #70218 with the full description and deployment context. Anything needed from my side to move this from keep_open toward a merge — regression test, rebase, or a split?