Skip to content

fix(mcp): serialize keepalive probes with RPC calls - #64989

Closed
TurgutKural wants to merge 1 commit into
NousResearch:mainfrom
TurgutKural:fix/mcp-keepalive-rpc-lock
Closed

fix(mcp): serialize keepalive probes with RPC calls#64989
TurgutKural wants to merge 1 commit into
NousResearch:mainfrom
TurgutKural:fix/mcp-keepalive-rpc-lock

Conversation

@TurgutKural

Copy link
Copy Markdown
Contributor

Summary

Serialize MCP keepalive RPCs with the per-server _rpc_lock used by normal tool calls.

Problem

MCPServerTask._keepalive_probe() sent ping and the list_tools fallback without acquiring the lock. On stateful JSON-RPC transports, a keepalive request could therefore race an in-flight tools/call response and corrupt the stream, causing false keepalive failures and reconnects.

Fix

  • Guard session.send_ping() with _rpc_lock.
  • Guard the session.list_tools() fallback with _rpc_lock.
  • Add regression tests proving both probes wait while another RPC owns the lock.

Validation

  • tests/tools/test_mcp_tool.py: 214 passed
  • Related MCP discovery/stability/reconnect tests: 255 passed total
  • py_compile: passed
  • git diff --check: passed

Scope

This is a targeted serialization fix. Existing reconnect, circuit-breaker, and optional-ping fallback behavior is unchanged.

@alt-glitch alt-glitch added type/bug Something isn't working comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint tool/mcp MCP client and OAuth P2 Medium — degraded but workaround exists sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state duplicate This issue or pull request already exists labels Jul 15, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Duplicate of #62811 — same mechanism: guards the MCP _keepalive_probe (send_ping + list_tools fallback) with the per-server _rpc_lock so it can't race an in-flight tools/call. #62811 is the earliest open PR with this fix (and is itself a narrow current-main salvage of the serialization invariant from the broader #48069). Consolidating on #62811; the reconnect/orphan-cancellation superset lives in #48069.

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Verdict: Approved

Looks Good

  • Fix(mcp): serialize keepalive probes with RPC calls
  • 45 additions, 2 deletions — targeted fix
  • No issues detected

Reviewed by Hermes Agent

@TurgutKural

Copy link
Copy Markdown
Contributor Author

Closing this duplicate in favor of a follow-up PR based on #62811.

#62811 covers the keepalive/active-RPC serialization race. The follow-up adds transport-aware recovery for empty-message ClosedResourceError/BrokenPipeError failures observed after the stdio session closes, plus regression coverage.

Follow-up will preserve this PR's _rpc_lock serialization and target the same MCP failure class.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint duplicate This issue or pull request already exists P2 Medium — degraded but workaround exists sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state tool/mcp MCP client and OAuth type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants