Skip to content

Python: Fence agent_server-history conversation turns with a pre-dispatch claim - #9737

Open
Darren Cohen (dargilco) wants to merge 3 commits into
microsoft:mainfrom
dargilco:dargilco/replay-of-tool-approval
Open

Darren Cohen (dargilco) wants to merge 3 commits into
microsoft:mainfrom
dargilco:dargilco/replay-of-tool-approval

Conversation

@dargilco

@dargilco Darren Cohen (dargilco) commented Oct 26, 2026 •

Copy link
Copy Markdown
Member

Motivation & Context

With the default history_source="agent_server", ResponsesHostServer did not establish exclusive ownership of a named conversation before dispatching a turn. The pre-dispatch conversation claim only applied to the service/agent history modes, and AgentServer serializes same-conversation turns only when its task subsystem is enabled (not the default). Two concurrent requests presenting the same pending mcp_approval_response to one conversation could therefore each load a stale copy of the session, each consume the same pending approval, and each dispatch the approved tool. The stale-session conflict was only reported after both tool effects had already happened.

Description & Review Guide

  • What are the major changes?
    • _responses.py: apply the existing conditional conversation claim write to agent_server history as well, so a losing concurrent turn fails before agent.run() and never reaches a tool.
    • Because agent_server history mutates no provider thread, a turn that fails or is cancelled releases its claim (_release_conversation_claim) from a shielded finally, leaving the conversation usable. A claim owned by another response is never touched. A non-approval turn restores the pre-turn state; an approval turn (which may already have run its tool) persists the consumed approval instead, so a retry cannot repeat the tool effect.
    • The claim is skipped when steering owns admission (AgentServer already serializes turns and lets a newer turn supersede an in-flight one).
    • The busy-conversation error for agent_server history no longer refers to a provider thread.
    • Tests: new regression test (a replayed approval dispatches once under concurrency), plus claim release on failure, claim release on cancellation, and foreign-claim preservation. _ConflictingConversationStore now rejects only the commit write, since a conflict is now detected at the claim.
  • What is the impact of these changes?
    • Concurrent same-conversation turns on the default history path now fail pre-dispatch with the existing "Another request advanced this agent session" / "in-flight turn" failures instead of both running. Stored, named-conversation turns incur one extra conditional session write.
    • A hard process crash mid-turn leaves the conversation claimed, so callers must start a new conversation (the same behavior the service path already documents).
  • What do you want reviewers to focus on?
    • Whether releasing the claim on failure/cancellation, and persisting consumed approval state for approval turns, is the right trade-off for agent_server history (vs. leaving it claimed as the service path does).
    • The interaction with steering (steering_owns_admission) and the claim-release ordering in the finally block.

Related Issue

N/A - no tracked issue yet; this is a correctness fix for a concurrency defect in the default Responses host path.

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: implementation, tests, and this PR description were drafted with GitHub Copilot.

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
  • 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.

…atch claim

Two concurrent requests replaying the same approval to one named conversation
could each load a stale session copy, consume the same pending approval, and
dispatch the approved tool twice. The pre-dispatch conversation claim was
skipped for history_source="agent_server" (the default), and AgentServer only
serializes same-conversation turns when its task subsystem is enabled.

Apply the conditional claim write to agent_server history too, so a losing turn
fails before agent.run(). Because no provider thread is mutated in this mode,
release the claim when a turn fails or is cancelled and restore the pre-turn
state. Skip the claim when steering already owns admission.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 26, 2026 23:37
@agent-framework-automation agent-framework-automation Bot added the python Usage: [Issues, PRs], Target: Python label Oct 26, 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.

🟡 Changes recommended

Failure cleanup can enable repeated tool effects, while cancellation during persistence can bypass claim release.

2 open issues
What changed in this PR

This PR extends pre-dispatch conversation claims to the default agent_server history mode, preventing concurrent turns from dispatching tools using stale session state.

Changes:

  • Claim stored, named conversations before agent execution, except when steering controls admission.
  • Release owned claims after uncommitted turns and clarify busy-conversation errors.
  • Add concurrency, failure, cancellation, and claim-ownership regression tests.
File Description
python/​packages/​foundry_hosting/​tests/​test_responses.py Adds claim lifecycle tests and updates commit-conflict simulation.
python/​packages/​foundry_hosting/​agent_framework_foundry_hosting/​_responses.py Extends admission claims and adds claim-release handling.

Review effort: Balanced

Comment thread python/packages/foundry_hosting/agent_framework_foundry_hosting/_responses.py Outdated
Comment thread python/packages/foundry_hosting/agent_framework_foundry_hosting/_responses.py Outdated

@github-actions github-actions Bot 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.

MAF Automated Review — Iteration 1

Result: Findings reported
Scope: full PR (1 commit(s)): 47645fd503f3
Model: gpt-5.6-sol

Overview

This PR extends the existing conditional session claim to stored agent_server conversations and adds ownership-checked release for uncommitted turns. The production Foundry store's ETag writes, pre-dispatch busy checks, and concurrency/ownership regressions provide strong guards against simultaneous approval dispatch. One cancellation window remains: a remotely committed pre-dispatch claim can be stranded because release eligibility is recorded only after the claim-write await returns.

Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
1 verified finding remained after source verification (1 medium) across 1 file. Details are attached to the affected lines below.

Affected areas: python/packages/foundry_hosting/agent_framework_foundry_hosting/_responses.py

Comment thread python/packages/foundry_hosting/agent_framework_foundry_hosting/_responses.py Outdated
- Do not reopen a consumed approval: an approval turn that fails after its tool
  may have run now persists the consumed state (without the claim) instead of
  restoring the pre-turn snapshot, so a retry cannot repeat the tool effect.
  Other turns still restore the pre-turn state.
- Release the claim from a finally clause, shielded, so cancellation during the
  response-snapshot write no longer strands the conversation as in-flight.
- Record the releasable claim before awaiting the claim write so a cancellation
  after the backend commits it can still be cleaned up (the release is
  ownership-checked).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

This branch was successfully deployed

1 active deployment
github-app-auth — 115c1f4f Deployed Oct 27, 2026 by dargilco via add_label #26241
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.

2 participants