[agentserver-responses] Exclude failed response inputs from replayable conversation history - #49009
prasanna164-code wants to merge 3 commits into
Conversation
|
Thank you for your contribution prasanna164-code! We will review the pull request and get back to you soon. |
|
Azure Pipelines: Successfully started running 1 pipeline(s). 7 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🟡 Changes recommended
File-store status indexes can become stale or diverge from response envelopes, causing incorrect history replay.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Prevents failed response items from poisoning subsequent replayable history.
Changes:
- Defines shared failed-status replay rules.
- Applies filtering to memory and file stores, with persisted status indexes.
- Adds unit, contract, and changelog coverage.
File summaries
| File | Description |
|---|---|
CHANGELOG.md |
Documents the history fix. |
store/_base.py |
Defines the provider invariant. |
store/_history.py |
Adds shared status normalization and filtering. |
store/_memory.py |
Filters failed response items in memory. |
store/_file.py |
Filters failed items using indexed status. |
tests/unit/test_failed_response_history.py |
Tests provider behavior and compatibility. |
tests/contract/test_failed_response_history.py |
Tests synchronous, streaming, and background flows. |
Review details
Suppressed comments (1)
sdk/agentserver/azure-ai-agentserver-responses/azure/ai/agentserver/responses/store/_file.py:744
Noneis both the default sentinel and a valid unknown status, so an update that omits/clearsstatusleaves the previous indexed value intact. After updating a failed response without a status, the in-memory provider treats it as replayable, but the file provider remains non-replayable. Always rewrite the indexed status on an envelope update so it stays aligned with the source envelope.
if status is not None:
current["status"] = normalize_status(status)
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
The hosted Foundry provider still delegates to an endpoint that does not demonstrably enforce the new invariant.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Balanced
AgentServer stores a response and its input items when it processes the initial response.created event. When the handler later fails the response, get_history_item_ids() still returned that response's input item IDs for every later request in the same conversation, and for requests chained through previous_response_id. The input that made the turn fail (for example a function_call_output with no matching call) was replayed into each subsequent request, which then failed the same way. Define the invariant on ResponseProviderProtocol and enforce it in the in-memory and file providers: a failed response contributes only the history it inherited; its own input and output items are excluded, before the history limit is applied. The failed response and its input items remain stored and retrievable through the response and input_items endpoints. The file store now records the response status in indexes.json on create and update so history resolution does not open every envelope; stores written before this change fall back to the envelope status. Fixes Azure#48929
Keeping a copy of the status in indexes.json created a second source of truth that could disagree with the envelope if the process stopped between the two atomic writes, and an update that omitted the status left the old indexed value in place. Read the status from the persisted envelope instead, which is written atomically, and drop the indexed copy.
431203d to
43dc68b
Compare
Description
Part of #48929 (provider-side fix; the hosted Foundry storage endpoint is tracked separately, see below).
When a turn fails, AgentServer already holds a stored response for it together with its
input_items.get_history_item_ids()then keeps returning those input IDs for every later request in the sameconversation_id, and for any request chained throughprevious_response_id. The input that made the turn fail (for example afunction_call_outputwith no matching call) is replayed into each subsequent request, which fails the same way, so one bad request poisons the conversation indefinitely. The Agent Framework workaround (microsoft/agent-framework#7637) was closed in favour of fixing this in the provider layer, where the issue notes the behaviour is defined.Change
The provider invariant suggested in the issue is now defined on
ResponseProviderProtocol.get_history_item_idsand enforced by the in-memory and file providers:failedcontributes only the history it inherited. Its own input items and output items are excluded from replayable history. The filter is applied while the chain is resolved, so it runs before thelimittruncation and failed items never displace replayable ones.completed,incomplete,cancelled, in-flight, or unknown) is unchanged.GET /responses/{id}andGET /responses/{id}/input_itemsfor diagnostics.store/_history.py(is_replayable_status), so both providers stay in lock-step.Both providers read the status from the stored response envelope, which is the single source of truth for it. For
FileResponseStorethat means one extra small JSON read per response in the chain, and no second copy of the status that could drift from the envelope after a crash between two writes. Existing on-disk stores need no migration.Output items are excluded along with the input because a failed turn may have produced a
function_callthat never received an output; replaying it would fail the next turn for the mirror-image reason.Known limitation
This only affects
previous_response_idchaining. When a response is chained, the history resolved for it is stored with it as a snapshot (history_item_ids). If a client chains to a response that is still running and that response later fails, the snapshot already holds the failed turn's input, so turns chained after it still receive it. Conversations are not affected: no snapshot is stored for them and their history is resolved from the store on every request, so a failed turn's input stops being replayed as soon as it has failed, even when another turn started while it was running.This can be closed inside the providers without changing the protocol: when a response is created, record the earlier turns in its chain that had not finished yet, and at resolution time drop their items from the snapshot if they ended up failed. I have this working locally for both providers. It depends on the stored response carrying
previous_response_id, which is set when the handler builds its event stream from the request (as the handler guide shows) but not otherwise, so I'd rather propose it as a follow-up; happy to open it if that direction works for you.Hosted Foundry storage
FoundryStorageProviderdelegates history resolution to the hostedhistory/item_idsendpoint, and hosted mode auto-selects that provider. The rule cannot be enforced client-side there: the storage API exposes per-responseGET, batch item retrieval andhistory/item_ids(bare item IDs), with no conversation listing and no item ownership, so the client cannot tell which returned IDs belong to a failed response. The invariant is therefore defined onResponseProviderProtocoland called out onFoundryStorageProvider.get_history_item_idsas the contract the hosted endpoint needs to implement (or expose a server-side filter for), as the issue suggests. This PR does not close #48929 on its own; the hosted part stays open with the service team.Testing
tests/unit/test_failed_response_history.pyruns every scenario against bothInMemoryResponseProviderandFileResponseStoreand asserts identical results:in_progressand later updated tofailed(the orchestrator's create-then-terminal-update path);previous_response_idchaining from a failed response yields only its inherited history, and a successful response chained after it stays clean;limittruncation;get_input_itemson the failed response still returns its items;indexes.jsoncannot make a failed turn replayable again;tests/contract/test_failed_response_history.pydrivesResponsesAgentServerHostend to end with the reproduction from the issue (afunction_call_outputfor a call that does not exist): the next valid turn in the conversation succeeds and its handler sees no history from the failed turn, successful turns before and after the failure are kept, chaining through the failed response viaprevious_response_idis clean, and the same holds for streaming and background requests. The failed response and its input items are still retrievable.With the status rule neutralised (old behaviour), 13 of the 22 new tests fail, including all end-to-end conversation, chaining, streaming and background cases. The existing unit and contract suites pass with the change.
black,pylintwith the repository guidelines checker,mypyandcspellare clean on the changed files.All SDK Contribution checklist:
General Guidelines and Best Practices
Testing Guidelines