Skip to content

Python: fix(python): prevent duplicate MCP tool execution after ambiguous connection loss - #9247

Open
Suhas Aggarwal (suhasagg) wants to merge 3 commits into
microsoft:mainfrom
suhasagg:investigate-mcp-duplicate-execution
Open

Suhas Aggarwal (suhasagg) wants to merge 3 commits into
microsoft:mainfrom
suhasagg:investigate-mcp-duplicate-execution

Conversation

@suhasagg

@suhasagg Suhas Aggarwal (suhasagg) commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

Motivation & Context

MCP tools/call requests may produce non-idempotent side effects, such as creating resources, modifying persistent state, triggering external workflows, or initiating transactions.

The existing Python MCP tool implementation can automatically retry a failed tools/call after certain connection failures. However, a connection failure does not establish whether the remote MCP server executed the original request.

For example:

  1. The client sends a tools/call request.
  2. The remote MCP server executes the requested operation.
  3. The connection terminates before the response reaches the client.
  4. The client interprets the failure as a transport error.
  5. The client automatically reissues the same request.
  6. The remote operation may execute twice.

This creates a potential duplicate-execution and data-integrity risk for state-changing MCP tools.

The fundamental problem is that a client cannot reliably distinguish between:

  • A request that never reached the server.
  • A request that reached the server but was not executed.
  • A request that executed successfully but whose response was lost.

The safe default is therefore to avoid automatically replaying an operation whose execution outcome is unknown.

MCP annotations such as idempotentHint and readOnlyHint are advisory metadata supplied by the remote server. They are not a reliable guarantee of replay safety, particularly when tools are misconfigured or implemented by third parties.

This PR addresses the duplicate-execution risk described in #9204 by removing automatic replay of ambiguous MCP tool calls and surfacing an explicit unknown-outcome error to the caller.

Description & Review Guide

What are the major changes?

1. Replace automatic tool-call retries with single-attempt execution

  • Rename _call_tool_with_retries() to _call_tool_once().
  • Remove the retry loop surrounding ClientSession.call_tool().
  • Execute each requested MCP tool invocation through a single client-side call_tool() attempt.
  • Avoid automatically repeating the same operation after connection loss.

This establishes a conservative no-replay policy for MCP tool execution.

2. Explicitly report ambiguous execution outcomes

For connection failures represented by ClosedResourceError or MCP session-termination errors:

  • Do not replay the original tools/call.
  • Raise ToolExecutionException explaining that the remote execution outcome is unknown.
  • Preserve the underlying exception using inner_exception and exception chaining.
  • Emit a warning describing the duplicate-execution risk.
  • Retain OpenTelemetry error reporting.

The error intentionally does not claim that the remote operation failed, because the server may already have completed it.

3. Preserve ordinary MCP error handling

  • Continue processing successful MCP responses.
  • Preserve the handling of MCP tool results containing isError=True.
  • Continue wrapping ordinary MCP exceptions appropriately.
  • Preserve the existing error-reporting and tracing behavior.

4. Add regression tests for ambiguous connection failures

The regression coverage verifies that:

  • An ambiguous connection failure does not trigger another call_tool() invocation.
  • The original connection exception is preserved.
  • The client does not automatically reconnect and replay the failed operation.

5. Verify advisory MCP annotations cannot authorize replay

Add parameterized tests covering discovered MCP tools advertising:

  • idempotentHint=True
  • readOnlyHint=True
  • Both hints enabled

The tests use MCP tool discovery through list_tools() and verify that an ambiguous connection failure does not cause the remote operation to be invoked again.

These tests deliberately treat advisory annotations as insufficient evidence of replay safety.

6. Update connection-reset integration expectations

Update the existing connection-reset integration test to:

  • Expect an unknown-outcome exception for the ambiguous failed invocation.
  • Verify that the original invocation is not automatically replayed.
  • Explicitly reconnect before initiating a separate operation.

What is the impact of these changes?

Safety improvement

The framework no longer automatically reissues an ambiguous MCP tools/call request through the removed retry mechanism.

This reduces the risk of duplicate side effects for tools that modify external state.

Explicit failure semantics

Callers receive an exception indicating that the remote execution outcome is unknown, rather than receiving an implicit retry that may duplicate the operation.

Behavioral compatibility

This intentionally changes the previous automatic-retry behavior for affected MCP tool calls.

Applications relying on automatic replay after connection loss may need to handle the exception and explicitly reconnect before a subsequent independent operation.

Recovery scope

This PR does not implement automatic session recovery after an ambiguous tool-call failure.

The existing explicit connect(reset=True) mechanism remains available for reconnecting before a separate operation.

Framework-owned session recovery, caller-managed session boundaries, and concurrent in-flight operations require careful lifecycle handling. Those concerns are not claimed as solved by this PR.

The implementation focuses on establishing a safe no-replay baseline without introducing additional reconnection races.

What do you want reviewers to focus on?

I would particularly appreciate maintainer feedback on the following design decisions:

1. Conservative no-replay policy

Is removing automatic replay from tools/call the preferred baseline when the client cannot determine whether the remote server executed the request?

2. Treatment of MCP tool annotations

The implementation deliberately does not use idempotentHint or readOnlyHint to authorize replay after an ambiguous connection failure.

This avoids relying on remote advisory metadata as a guarantee of execution safety.

3. Unknown-outcome error semantics

Does ToolExecutionException with an explicit unknown-outcome message and preserved underlying exception provide an appropriate caller-facing contract?

4. Recovery separation

Would maintainers prefer to keep safe recovery of framework-owned sessions for subsequent independent calls as a follow-up, or include that functionality in this PR?

Any such recovery should avoid replaying the original failed operation and respect caller-managed session ownership and concurrent operations.

5. Compatibility and test coverage

Please review the change in retry behavior and whether additional transport-failure or lifecycle tests would be useful before merging.

Related Issue

Fixes #9204

This PR provides an implementation focused on a strict no-replay policy for ambiguous MCP tool-call failures.

The key distinction is that this implementation does not authorize automatic replay based on advisory idempotentHint or readOnlyHint values, including when those hints are advertised during MCP tool discovery.

The intent is to provide a conservative safety baseline rather than to duplicate or supersede another contributor's work without review.

I welcome maintainer guidance on which approach best fits the framework's desired execution semantics, including whether elements of the approaches should be combined.

Validation

Full MCP test suite

Executed from the python directory:

uv run --group dev pytest packages/core/tests/core/test_mcp.py -q

Results:

  • 398 passed
  • 2 skipped
  • 0 failed

Targeted regression tests

uv run --group dev pytest packages/core/tests/core/test_mcp.py \
  -k "untrusted_annotations_never_authorize_replay or does_not_retry_after_connection_loss" -v

Results:

  • 4 passed
  • 0 failed

Lint and formatting

uv run --group dev ruff check \
  packages/core/agent_framework/_mcp.py \
  packages/core/tests/core/test_mcp.py

uv run --group dev ruff format --check \
  packages/core/agent_framework/_mcp.py \
  packages/core/tests/core/test_mcp.py

git diff --check

All checks passed.

The full MCP suite reported warnings, including experimental/deprecated API warnings and a coroutine warning in an existing test. No test failures occurred.

AI Assistance

  • No material AI assistance was used.
  • This is an AI-assisted contribution. I reviewed, understood, and verified all submitted content and accept responsibility for it.

AI assistance details:

AI assistance was used for implementation, tests. All changes were reviewed and tested locally.

Contribution Checklist

  • The code builds clean without any errors or warnings
  • All unit tests pass, and I have added new tests where possible
  • The PR follows the [Contribution Guidelines](https://github.com/microsoft/agent-framework/blob/main/CONTRIBUTING.md)
  • This PR links to an agreed issue with no competing open PR, or the Related Issue section documents a trivial-change or repository-automation exception.
  • This is not a breaking change. If it is a breaking change, add the breaking change label (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.

Copilot AI balanced review requested due to automatic review settings October 10, 2026 10:49
@agent-framework-automation agent-framework-automation Bot added the python Usage: [Issues, PRs], Target: Python label Oct 10, 2026
@github-actions github-actions Bot changed the title fix(python): prevent duplicate MCP tool execution after ambiguous connection loss Python: fix(python): prevent duplicate MCP tool execution after ambiguous connection loss Oct 10, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The safety behavior and regression coverage are sound; only a non-blocking stale test docstring remains.

1 open finding
What changed in this PR

Prevents duplicate MCP tool execution after ambiguous connection loss by removing automatic replay.

Changes:

  • Replaces retrying tool calls with single-attempt execution.
  • Reports unknown outcomes while preserving underlying exceptions and telemetry.
  • Adds regression coverage for connection loss and advisory annotations.
File Description
python/​packages/​core/​agent_framework/​_mcp.py Implements no-replay behavior and unknown-outcome errors.
python/​packages/​core/​tests/​core/​test_mcp.py Updates recovery expectations and adds regression tests.

🧠 Review effort: Balanced


💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

Comment thread python/packages/core/tests/core/test_mcp.py

This branch was successfully deployed

1 active deployment
github-app-auth — ced5913c Deployed Oct 11, 2026 by suhasagg via add_label #24925
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

python Usage: [Issues, PRs], Target: Python

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Python: [Bug]: MCP tools/call retry can duplicate non-idempotent side effects after connection loss

2 participants