Repository navigation
Python: add background tool recovery for failed runs - #10110
sophia-ramsey wants to merge 4 commits into
Conversation
GitHub Actions
Inactive
GitHub Actions
Inactive
agent-framework-automation
Bot
added
documentation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Code coverage overviewLanguages: Python Python / code-coverage/python
View coverage of the 6 most impacted files.
ℹ️ Calculations represent line coverage. Results may change as calculations are updated. Updated |
There was a problem hiding this comment.
🟡 Changes recommended
Request Shared State can overwrite recovery checkpoints and the handled-token ledger, erasing pending results and defeating replay protection.
6 open issues
What changed in this PR
Adds session-backed recovery for Python background tool calls, addressing #9031 by separating completed tool execution from result delivery so supported retries avoid repeating side effects.
Changes:
- Records execution claims, completed results, delivery checkpoints, and invocation budgets in a reusable
AgentSession. - Integrates recovery into streaming and non-streaming function loops, with provider-scoped state and rejection of unsafe retries.
- Adds regression coverage, documents recovery boundaries, and explicitly closes interrupted sample streams.
| File | Description |
|---|---|
python/samples/02-agents/background_responses.py |
Closes the interrupted stream before resumption. |
python/packages/openai/tests/openai/test_openai_chat_client.py |
Tests retries, restored sessions, concurrency, limits, and approvals. |
python/packages/openai/AGENTS.md |
Documents adapter recovery responsibilities and session requirements. |
python/packages/openai/agent_framework_openai/_chat_client.py |
Defines recovery scope and provider-owned state. |
python/packages/core/AGENTS.md |
Adds the recovery module to the package map. |
python/packages/core/agent_framework/_tools.py |
Integrates execution tracking and delivery recovery into both function loops. |
python/packages/core/agent_framework/_tool_recovery.py |
Implements recovery state, checkpoints, and replay protection. |
docs/decisions/0049-python-background-tool-result-recovery.md |
Describes the proposed design, lifecycle, and persistence responsibilities. |
Review effort: Balanced
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
MAF Automated Review — Iteration 1
Result: Findings reported
Scope: full PR (1 commit(s)): 9fd47927c247
Model: gpt-5.6-sol
Overview
The PR introduces a session-backed recovery state machine that claims tool calls before execution, checkpoints completed results separately from delivery, and restores transcript and budget state without rerunning tool bodies. Its tests and fail-closed guards cover retryable transport errors, stale or corrupt state, partial execution, and concurrent use. Residual risk remains around inherited Foundry identities and state ownership, premature submission-state transitions, empty-session scope binding, and an intentional behavioral break for stateless callers.
Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
5 verified findings remained after source verification (5 medium) across 3 files. Details are attached to the affected lines below.
Affected areas: python/packages/core/agent_framework/_tool_recovery.py, python/packages/core/agent_framework/_tools.py, python/packages/openai/agent_framework_openai/_chat_client.py
|
sophia-ramsey Documentation direction: remove |
GitHub Actions
Inactive
|
still a WIP but welcome to comments/feedback, I think more fixes will be needed if we want to fully support this functionality with AG-UI |
|
sophia-ramsey, I found one material issue on the current head:
|
| if self._state.pending is not None: | ||
| pending = self._state.pending | ||
| if pending.status == "submitting": | ||
| self._remember(pending.token, pending.response_id) |
There was a problem hiding this comment.
sophia-ramsey This records the delivery only after the whole chat-middleware invocation reaches observe_response. MessageInjectionMiddleware can issue another provider request in that same invocation. If the tool-output submission succeeds but the later injected-message request fails, recovery marks the original delivery ready and can resubmit the already accepted tool output against the earlier response ID. That duplicates provider-side processing and branches the continuation. Please persist delivery acceptance and advance continuation at each provider-response boundary before middleware can issue another request, and add a regression for a successful delivery followed by a failed injected request.
| if recovery is None: | ||
| return ResponseStream(updates, finalizer=finalize_stream) | ||
|
|
||
| async def run_stream() -> AsyncIterable[ChatResponseUpdate]: |
There was a problem hiding this comment.
sophia-ramsey, this persists busy=True before the stream yields, but ownership is released only when the stream is closed or finalized. If a caller breaks out of async for and immediately resumes on the same session, the old generator may not be finalized yet, so the resume fails; saving the session in between can make the busy state durable. Can we preserve the prior break-and-resume behavior or release abandoned stream ownership automatically, with a regression that does not require an explicit stream.close()?
| pending = state.pending | ||
| if self._token != pending.token or self._inputs != pending.inputs: | ||
| raise ChatClientInvalidRequestException( | ||
| "Pending tool results must be recovered with the original token and no new input." |
There was a problem hiding this comment.
sophia-ramsey, for an initial streaming background run whose tool-output submission fails with 429/503, the pending state keeps both the original inputs and the held token. Recovery then rejects a token-only poll and also rejects repeating the original call; only supplying both together works, despite the error saying to use the original token with no new input. Please make the supported recovery call unambiguous and add a regression for the initial-stream path.
Motivation & Context
An OpenAI background poll can complete a local tool and then fail while submitting its result. Retrying the original continuation token can execute that tool again, repeating side effects such as sending an email. This change separates completed tool execution from result delivery in both streaming and non-streaming background polling.
Description & Review Guide
Adds session-backed
BackgroundToolRecoveryto retain execution claims, completed results, prepared delivery messages, and invocation budgets. The shared function loop resumes result delivery without retrieving or executing completed calls again, while the OpenAI adapter scopes recovery to the provider configuration. Regression tests cover retries, restored sessions, stale tokens, partial execution, concurrency, and existing approval behavior.Supported retries reuse completed results without rerunning tool bodies or function middleware, while unsafe or indeterminate recovery attempts fail explicitly. Automatic background tool execution now requires an explicit, reusable
AgentSession, so stateless callers must create and retain one across polls. Durable session persistence and coordination between workers remain application responsibilities; this does not provide distributed exactly-once execution.Design decision and approach with the creation of
BackgroundToolRecoveryas well as how different states are handled. Details are in `docs/decisions/0049-python-background-tool-result-recovery.md'. Please let me know if there are any risks that I have not considered.Related Issue
Fixes #9031
AI Assistance
AI assistance details: investigation, planning, implementation, testing
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.