Python: omit failed Foundry turns from conversation chat history - #7637
Conversation
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
agent-framework-automation
Bot
added
the
python
GitHub Actions
Inactive
|
@microsoft-github-policy-service agree |
GitHub Actions
Inactive
|
Unfortunately this doesn't fix the issue, as the problem is that the messages get saved to the chat history, not the session. |
GitHub Actions
Inactive
|
You're right — the first commit skipped the wrong store. The agentserver response provider was persisting input items for failed turns, so the next request on the same conversation replayed them via chat history (get_history()), not the MAF session. 24ebe8b wraps the response store so failed turns persist without input items (including the in_progress → failed update path). A follow-up request on the same conversation no longer sees the bad function_call_output. Tests cover both the store wrapper and the HTTP conversation repro from the issue. |
GitHub Actions
Inactive
Shikhar Goel (sgoel2be24-cyber)
changed the title
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
9c43deb to
4b6b97e
Compare
GitHub Actions
Inactive
Python Test Coverage Report •
Python Unit Test Overview
|
||||||||||||||||||||||||||||||
| async for event in events: | ||
| buffered.append(event) | ||
| except Exception as ex: | ||
| handler_error = ex | ||
| failed = handler_error is not None or any( | ||
| _response_field(event, "type") == "response.failed" for event in buffered | ||
| ) | ||
| if store and failed: | ||
| self._failed_sync_response_ids.add(response_id) | ||
| for event in buffered: | ||
| yield event | ||
| if handler_error is not None: | ||
| raise handler_error |
There was a problem hiding this comment.
this concerns me, because we are first exhausting the stream and then checking and yielding, I would prefer if we can yield directly because this will slow down the whole response.
There was a problem hiding this comment.
Thanks—agreed that exhausting the handler stream before yielding is undesirable.
The constraint I ran into is that agentserver persists the input items during the initial response.created / in_progress write, while the terminal failure is only known later. ResponseProviderProtocol.update_response() cannot atomically clear those input references. Yielding directly with the current wrapper would therefore reintroduce the failed-conversation-history bug, and the earlier delete-then-create approach was removed because it was non-atomic.
Would you prefer this behavior to be fixed in azure-ai-agentserver-responses—for example, by atomically excluding or clearing input references for failed responses—or should the provider protocol expose an operation that this host can use?
I will address the docstring and concrete typing comments regardless.
There was a problem hiding this comment.
Updated after #7997 merged: 8213cc6 respects the new history_source boundary. Agent-owned history keeps the original response provider and yields without this buffer; failed-input filtering/buffering is limited to regular agents using AgentServer history. Regression tests verify both history modes and that agent mode bypasses buffering. The default AgentServer-history path still buffers synchronous requests, so I am leaving this thread open. The merged history-source change does not add an atomic input-reference update API. Should failed-input exclusion for that path live in azure-ai-agentserver-responses, or should its provider protocol gain the atomic operation discussed above?
|
/review |
|
Shikhar Goel (@sgoel2be24-cyber) please fix the merge conflict, and there is a update coming to the way we handle storage in #7997 I suggest updating this after that one merged |
4b6b97e to
79aa6e3
Compare
GitHub Actions
Inactive
|
Thanks Eduard van Valkenburg (@eavanvalkenburg). The merge conflict is fixed: rebased onto main (1a51fdd) and pushed 79aa6e3, including the prepared docstring and concrete-type cleanup. Foundry Hosting validation passes: 276 unit tests, 90% package coverage, Ruff, strict Pyright, and all five test type-checkers. The two cleanup threads are resolved; the streaming/storage thread remains open. I will revisit that design after #7997 merges, as suggested. |
79aa6e3 to
8213cc6
Compare
GitHub Actions
Inactive
|
Follow-up: #7997 merged while I was validating the earlier update. I have now rebased onto it (1f7f4b3) and pushed 8213cc6. Failed-input filtering is scoped to regular agents using AgentServer history; agent-owned history retains normal protocol storage and bypasses buffering. Added regression coverage for both modes and updated the resilient-store guard test for the new constructor validation order. All 294 Foundry Hosting unit tests pass, with 91% coverage; Ruff, strict Pyright, and all five test type-checkers pass. The docstring/type cleanup remains complete. The remaining default-mode buffering/API question is documented in the open review thread. |
|
/review |
![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 (3 commit(s)): 3fb9d59225fd, 5b4281fae1fc, 8213cc685bc4
Model: gpt-5.6-sol-fast
Overview
The change correctly lets AgentServer resolve its configured store before wrapping it and adds end-to-end coverage that failed synchronous conversation turns are not replayed. The history-source guards and successful marker-consumption path are well tested. Two residual defects remain: failed-ID state is shared across tenant contexts and can survive persistence failures, while the provider wrapper also removes inputs from standalone failed responses that cannot affect conversation history.
Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
2 verified findings remained after source verification (1 high, 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
| for event in buffered | ||
| ) | ||
| if store and failed: | ||
| self._failed_sync_response_ids.add(response_id) |
There was a problem hiding this comment.
response_id is client-controlled, but this process-wide set is not keyed by PlatformContext and the wrapper clears it only after create_response succeeds. A duplicate or transient storage failure therefore leaves a stale marker; another tenant or request reusing the ID then has its successful inputs silently dropped, while repeated failures accumulate unreclaimed entries. Please scope this state to the request/tenant and guarantee cleanup on every persistence exit, or avoid the shared side channel.
| """Persist ``response``, dropping input items when the turn failed.""" | ||
| response_id = response["id"] | ||
| known_failed = response_id in self._failed_response_ids | ||
| if _is_failed_stored_response(response) or known_failed: |
There was a problem hiding this comment.
This also drops inputs for a failed stored response with neither a conversation nor a previous-response chain. Such inputs cannot poison later history, so standalone /input_items retrieval now loses request and diagnostic data even though non-conversation runs are intended to remain unchanged. Please limit omission to responses that participate in replayable history, or filter history references separately while preserving standalone response inputs.
Motivation & Context
When a hosted Foundry agent is used with
conversation_id, a failed turn still stored its input items on the agentserver response/conversation store. The next request on that conversation replayed them viaget_history(). Azure OpenAI does not keep failed input on the conversation.Fixes #7630
Description & Review Guide
ResponsesAgentServerHostresolve and validate its configured/default response store before wrapping it, preserving hosted and local persistence defaults plus the resilient-background guard.python/packages/foundry_hosting/tests/test_responses.pycover the HTTP conversation repro, provider behavior, default persistent storage, and the resilience guard.Please focus on: failed synchronous turns stay out of
get_history(), successful turns still persist, non-conversation runs are unchanged, and the base host's storage behavior is preserved.Related Issue
Fixes #7630
Contribution Checklist
Assistance: drafted with an AI coding agent and reviewed before opening.